core: uncrustify by kaspar030 · Pull Request #10867 · RIOT-OS/RIOT

kaspar030 · 2019-01-25T11:11:40Z

Contribution description

This PR applies the current uncrustify-riot.cfg to core.
atomic_*.c are excluded due to too many misformattings in the macros.

Most of the changes are alright. My main pain point are string concatenations for inttype format macros, e.g.,

DEBUG("foo %"PRIu32"\n");

changes into

DEBUG("foo %" PRIu32 "\n");

(note the extra spaces between " and PRIu32.

But I can live with that.

Testing procedure

Check diff.

Issues/PRs references

miri64 · 2019-01-25T12:35:16Z

Most of the changes are alright. My main pain point are string concatenations for inttype format macros, e.g.,

DEBUG("foo %"PRIu32"\n");

changes into

DEBUG("foo %" PRIu32 "\n");

(note the extra spaces between " and PRIu32.

But I can live with that.

I actually find the auto-formatted version more readable, but again, a matter of taste.

kaspar030 · 2019-01-25T13:53:57Z

I actually find the auto-formatted version more readable, but again, a matter of taste.

Yup. I'm OK with the auto-formatted version, too.

kaspar030 · 2019-01-25T13:54:14Z

I realized I'd forgotten the includes... They're uncrustified now, too.

miri64

Arghs .... sometimes I forget that I actually have to submit my review... here an older comment of mine (from before I added #10867 (comment))

core/atomic_c11.c

miri64 · 2019-01-25T14:54:03Z

core/atomic_c11.c

 */
 bool __atomic_compare_exchange_c(size_t len, void *ptr, void *expected,
-    void *desired, bool weak, int success_memorder, int failure_memorder)
+                                 void *desired, bool weak, int success_memorder, int failure_memorder)


This was delivered by uncrustify this way? int failure_memorder could (and should) be on the next line.

It seems like our uncrustify config doesn't do line wrapping at all. I've pushed a change to make it do it.

It feels quite intrusive... ;)

It feels quite intrusive... ;)

If it formats it this way it's plain wrong though.

If it formats it this way it's plain wrong though.

Hmyeah. I tried limiting uncrustify's line wrap to 100 cols. It prevents this mishap.

An alternative is setting ls_func_split_full, which results in this (if a line is longer than the configured limit):

-bool __atomic_compare_exchange_c(size_t len, void *ptr, void *expected, - void *desired, bool weak, int success_memorder, int failure_memorder) +bool __atomic_compare_exchange_c(size_t len, + void *ptr, + void *expected, + void *desired, + bool weak, + int success_memorder, + int failure_memorder)

Don't like that style (and only use it if everything else doesn't make sense stylistically)

I've split out the 100char config change into another PR: #10873

stale · 2019-08-10T02:07:53Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

kaspar030 · 2019-08-19T21:02:40Z

rebased
cleaned first commit (contained both disabling of uncrustify and some formatting changes
re-run uncrustify with uncrustify: split lines at 80 chars #10873 (80char line limit) merged

@miri64 should we go forward with this?

miri64 · 2019-08-28T04:37:36Z

I do think so. I put it on my TODO list ;-).

kaspar030 · 2019-09-10T11:47:57Z

ping this is purely cosmetic. :)

miri64

ping this is purely cosmetic. :)

True, still found some nits though :-)

core/atomic_sync.c

core/bitarithm.c

core/include/kernel_defines.h

core/include/mbox.h

core/include/sched.h

core/panic.c

core/sched.c

miri64 · 2019-09-10T12:18:28Z

core/sched.c

+        "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?

miri64 · 2019-10-29T16:41:05Z

@kaspar030 ping?

kaspar030 · 2019-10-29T17:01:32Z

@kaspar030 ping?

Thanks for the heads up. I've addressed most issues and re-ran uncrustify.
There are some alignments in core/include/byteorder.h that seem broken. I'm about to add the ignore statements to the whole file. What do you think?

miri64 · 2019-10-29T17:03:27Z

I'm about to add the ignore statements to the whole file. What do you think?

If it is properly ended, go ahead :-).

miri64 · 2019-10-29T17:07:03Z

There are some alignments in core/include/byteorder.h that seem broken. I'm about to add the ignore statements to the whole file. What do you think?

Mhh.. after looking at the current proposal, I'm not sure what you mean. Can you point me to that?

core/include/byteorder.h

kaspar030 · 2020-03-10T12:51:46Z

@miri64 I've rebased this, it is otherwise unchanged. Let's find a shorter you-look-I-react cycle.

miri64

One small comment remaining

miri64 · 2020-03-10T15:51:47Z

core/include/sched.h

+                                                     `st >= STATUS_ON_RUNQUEUE`
+                                                 */


This looks weird. Any idea why uncrustify is doing this?

I think actually I did this, must be a merge leftover. Will fix.

done. Now uncrustify just realignes the second line a bit.

kaspar030 · 2020-03-30T15:06:04Z

fixed comment, rebased

miri64 · 2020-03-30T15:07:43Z

ACK! Let's finally merge this!

kaspar030 · 2020-03-30T19:24:08Z

Thanks @miri64!

miri64 reviewed Jan 25, 2019

View reviewed changes

core/atomic_c11.c Outdated Show resolved Hide resolved

miri64 reviewed Jan 25, 2019

View reviewed changes

kaspar030 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 25, 2019

kaspar030 force-pushed the uncrustify_core branch from 76daba2 to 1579d02 Compare January 25, 2019 15:42

kaspar030 mentioned this pull request Jan 25, 2019

uncrustify: split lines at 80 chars #10873

Merged

smlng added the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 5, 2019

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

kaspar030 force-pushed the uncrustify_core branch from 1579d02 to 05c6fd5 Compare August 19, 2019 20:59

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 19, 2019

kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Aug 19, 2019

miri64 requested changes Sep 10, 2019

View reviewed changes

kaspar030 force-pushed the uncrustify_core branch from 05c6fd5 to 1ba8940 Compare October 29, 2019 16:58

kaspar030 commented Oct 30, 2019

View reviewed changes

core/include/byteorder.h Show resolved Hide resolved

kaspar030 mentioned this pull request Mar 10, 2020

uncrustify: add auto uncrustify with blacklist #8519

Merged

kaspar030 force-pushed the uncrustify_core branch from 1ba8940 to b826707 Compare March 10, 2020 12:00

miri64 reviewed Mar 10, 2020

View reviewed changes

kaspar030 added 3 commits March 30, 2020 17:00

core: disable uncrustify for atomic_*.c

4a31068

core: make some formatting uncrustify friendly (or ignore)

f99bc89

core: uncrustify

8efe596

kaspar030 force-pushed the uncrustify_core branch from b826707 to 8efe596 Compare March 30, 2020 15:02

miri64 approved these changes Mar 30, 2020

View reviewed changes

kaspar030 merged commit 9de2ff7 into RIOT-OS:master Mar 30, 2020

kaspar030 deleted the uncrustify_core branch March 30, 2020 19:24

leandrolanzieri added this to the Release 2020.04 milestone Apr 3, 2020

jia200x mentioned this pull request Oct 26, 2020

codestyle: fix code style in loops #9245

Closed

miri64 mentioned this pull request Jan 19, 2021

treewide: re-uncrustify whitelisted files #15802

Merged

Conversation

kaspar030 commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Jan 25, 2019

Uh oh!

kaspar030 commented Jan 25, 2019

Uh oh!

kaspar030 commented Jan 25, 2019

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Aug 10, 2019

Uh oh!

kaspar030 commented Aug 19, 2019

Uh oh!

miri64 commented Aug 28, 2019

Uh oh!

kaspar030 commented Sep 10, 2019

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 29, 2019

Uh oh!

kaspar030 commented Oct 29, 2019

Uh oh!

miri64 commented Oct 29, 2019

Uh oh!

miri64 commented Oct 29, 2019

Uh oh!

Uh oh!

kaspar030 commented Mar 10, 2020

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Mar 30, 2020

Uh oh!

miri64 commented Mar 30, 2020

Uh oh!

kaspar030 commented Mar 30, 2020

Uh oh!

Reviewers

kaspar030 commented Jan 25, 2019 •

edited

Loading