[riot-notifications] [RIOT-OS/RIOT] core: uncrustify (#10867)

Martine Lenders notifications at github.com
Tue Sep 10 14:19:53 CEST 2019


miri64 requested changes on this pull request.

> _ping_ this is purely cosmetic. :)

True, still found some nits though :-)

> @@ -29,6 +29,11 @@
 #include <string.h>
 #include "irq.h"
 
+/*
+ * uncrustify mis-formats the macros in this file, so disable it globally.
+ * begin{code-style-ignore}

`/* end{code-style-ignore} */` missing?

> @@ -31,10 +31,10 @@ unsigned bitarithm_msb(unsigned v)
     register unsigned shift;
 
     r =     (v > 0xFFFF) << 4; v >>= r;
-    shift = (v > 0xFF  ) << 3; v >>= shift; r |= shift;
-    shift = (v > 0xF   ) << 2; v >>= shift; r |= shift;
-    shift = (v > 0x3   ) << 1; v >>= shift; r |= shift;
-                                            r |= (v >> 1);
+    shift = (v > 0xFF) << 3; v >>= shift; r |= shift;
+    shift = (v > 0xF) << 2; v >>= shift; r |= shift;
+    shift = (v > 0x3) << 1; v >>= shift; r |= shift;
+    r |= (v >> 1);

I remember that some people wanted the original formatting (I also think it looks quite nice, but it of course breaks any automation). Maybe surround it with `/* begin{code-style-ignore} */`/`/* end{code-style-ignore} */`?

> @@ -42,21 +42,21 @@
  */
 #if __STDC_VERSION__ >= 201112L
 #   define container_of(PTR, TYPE, MEMBER) \
-        (_Generic((PTR), \
-            const __typeof__ (((TYPE *) 0)->MEMBER) *: \
-                ((TYPE *) ((char *) (PTR) - offsetof(TYPE, MEMBER))), \
-            __typeof__ (((TYPE *) 0)->MEMBER) *: \
-                ((TYPE *) ((char *) (PTR) - offsetof(TYPE, MEMBER))) \
-        ))
+    (_Generic((PTR), \
+              const __typeof__ (((TYPE *)0)->MEMBER) * : \
+              ((TYPE *)((char *)(PTR)-offsetof(TYPE, MEMBER))), \
+              __typeof__ (((TYPE *)0)->MEMBER) * : \
+              ((TYPE *)((char *)(PTR)-offsetof(TYPE, MEMBER))) \
+              ))

This is somewhat hard to read, but I'm pretty sure the formatter broke some formatting here.

>  #elif defined __GNUC__
 #   define container_of(PTR, TYPE, MEMBER) \
-        (__extension__ ({ \
-            __extension__ const __typeof__ (((TYPE *) 0)->MEMBER) *__m____ = (PTR); \
-            ((TYPE *) ((char *) __m____ - offsetof(TYPE, MEMBER))); \
-        }))
+    (__extension__({ \
+        __extension__ const __typeof__ (((TYPE *)0)->MEMBER) * __m____ = (PTR); \
+        ((TYPE *)((char *)__m____ - offsetof(TYPE, MEMBER))); \
+    }))

Here as well.

>  #else
 #   define container_of(PTR, TYPE, MEMBER) \
-        ((TYPE *) ((char *) (PTR) - offsetof(TYPE, MEMBER)))
+    ((TYPE *)((char *)(PTR)-offsetof(TYPE, MEMBER)))

This definitely now is broken (`-` should have spaces around it)

> @@ -31,7 +31,8 @@ extern "C" {
 #endif
 
 /** Static initializer for mbox objects */
-#define MBOX_INIT(queue, queue_size) {{0}, {0}, CIB_INIT(queue_size), queue}
+#define MBOX_INIT(queue, queue_size) { { 0 }, { 0 }, CIB_INIT(queue_size), \
+        queue }

I think I prefer

```C
#define MBOX_INIT(queue, queue_size) { \
        { 0 }, { 0 }, CIB_INIT(queue_size), queue \
    }
```

> @@ -122,7 +122,7 @@ typedef enum {
  * @{
  */
 #define STATUS_ON_RUNQUEUE      STATUS_RUNNING  /**< to check if on run queue:
-                                                 `st >= STATUS_ON_RUNQUEUE`   */
+                                                   `st >= STATUS_ON_RUNQUEUE`   */

This seems off...

```suggestion
                                                     `st >= STATUS_ON_RUNQUEUE`   */
```

> @@ -41,13 +41,14 @@ const char assert_crash_message[] = "FAILED ASSERTION.";
 /* flag preventing "recursive crash printing loop" */
 static int crashed = 0;
 
-void __attribute__((weak)) panic_arch(void) {}
+void __attribute__((weak)) panic_arch(void)
+{}

:thinking: not sure this is more correct...

>  
-    DEBUG("sched_run: active thread: %" PRIkernel_pid ", next thread: %" PRIkernel_pid "\n",
-          (kernel_pid_t)((active_thread == NULL) ? KERNEL_PID_UNDEF : active_thread->pid),
-          next_thread->pid);
+    DEBUG(
+        "sched_run: active thread: %" PRIkernel_pid ", next thread: %" PRIkernel_pid "\n",
+        (kernel_pid_t)((active_thread ==
+                        NULL) ? KERNEL_PID_UNDEF : active_thread->pid),
+        next_thread->pid);

This looks pretty messed up. How about:

```C
    DEBUG(
        "sched_run: active thread: %" PRIkernel_pid ", next thread: %" PRIkernel_pid "\n",
        (kernel_pid_t)((active_thread == NULL)
                      ? KERNEL_PID_UNDEF
                      : active_thread->pid),
        next_thread->pid);
```

This should also be accepted by `uncrustify`

>  
-    DEBUG("sched_run: active thread: %" PRIkernel_pid ", next thread: %" PRIkernel_pid "\n",
-          (kernel_pid_t)((active_thread == NULL) ? KERNEL_PID_UNDEF : active_thread->pid),
-          next_thread->pid);
+    DEBUG(
+        "sched_run: active thread: %" PRIkernel_pid ", next thread: %" PRIkernel_pid "\n",
+        (kernel_pid_t)((active_thread ==
+                        NULL) ? KERNEL_PID_UNDEF : active_thread->pid),
+        next_thread->pid);

Also (but unrelated to this PR): Is the cast really necessary?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/10867#pullrequestreview-286099857
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190910/7b048754/attachment-0001.htm>


More information about the notifications mailing list