global: Run uncrustify on all .c source files. #14046

Gadgetoid · 2024-03-06T15:50:50Z

Enforce .c code consistency by running uncrustify -c tools/uncrustify.cfg across the whole* codebase, with:

find . -name *.c | xargs uncrustify -c tools/uncrustify.cfg --replace --no-backup

Followed by tools/codeformat.py to fix all the erroneous ifdef indents.

Skip third-party code in lib/ and drivers/cc3100/
Skip third-party code in ports/cc3200/FreeRTOS/
Skip all exclusions listed in tools/codeformat.py

As mentioned in #13777 it looks like there are files that are not covered either by the PATHS or EXCLUSIONS in tools/codeformat.py, resulting in some files that have not been considered for code consistency reformatting.

I think there are some valid changes here, but also a few left that raise an eyebrow.

Should some of these paths be added to the code formatting tool? Eg:

drivers/bus/*.[ch]
drivers/memory/*.[ch]
examples/**/*.[ch]

Also it looks like extmod/nimble/nimble/nimble_npl_os.c is missed, since the glob is extmod/nimble/*.[ch] and, as such, does not include that subdirectory.

Other files should either be formatted, or explicitly excluded.

The nrf port has been ignored because it's not fully formatted, but if it's ignored will it ever be fully formatted?

codecov · 2024-03-06T15:53:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.57%. Comparing base (1b89c50) to head (17e6da0).
Report is 36 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #14046   +/-   ##
=======================================
  Coverage   98.57%   98.57%           
=======================================
  Files         164      164           
  Lines       21335    21345   +10     
=======================================
+ Hits        21030    21040   +10     
  Misses        305      305

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-03-06T15:57:09Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

jimmo · 2024-03-07T05:57:38Z

Thanks @Gadgetoid

Should some of these paths be added to the code formatting tool?

Definitely.

The initial list of inclusions was partially just to keep the scope of the transition manageable.

We should add "drivers/**/*.[ch]" to the paths, and add an exclusion for drivers/cc3100.

Also add "examples/**/*.[ch]", "shared/libc/*.[ch]", and "shared/readline/*.[ch]".

And then finally the nimble glob should be updated to include a **.

Then I think this PR should just be the results of running that, no other changes (i.e. revert the changes to drivers/cc3100 and shared/memzip and ports/cc3200).

nRF we can sort out later (but replacing it with Zephyr is still the plan there...)

projectgus · 2024-03-07T23:46:22Z

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

Gadgetoid · 2024-03-08T12:42:59Z

@jimmo split into two commits, one to update the rules and one to apply them.

And, of course, rebased to avoid conflicts with the STATIC and whitespace fixes.

Gadgetoid · 2024-03-08T12:45:30Z

extmod/nimble/nimble/nimble_npl_os.c

-    );
+        #if MICROPY_MALLOC_USES_ALLOCATED_SIZE
+        , alloc->size
+        #endif


This is a bit of an odd case- looks like uncrustify bumps up the indent but the fixups don't catch it. Should they?

Looking at other examples I think the preference here would be to rewrite this code as follows:

#if MICROPY_MALLOC_USES_ALLOCATED_SIZE m_free(alloc, alloc->size); #else m_free(alloc); #endif

Gadgetoid · 2024-03-08T12:48:12Z

shared/readline/readline.c

-#endif
+                #if MICROPY_REPL_EMACS_EXTRA_WORDS_MOVE
+            backward_word:
+                #endif


The following addition to the fixup_c function catches these edge cases, but I don't love it-

diff --git a/tools/codeformat.py b/tools/codeformat.py index e2e5c0cc0..97b466d06 100755 --- a/tools/codeformat.py +++ b/tools/codeformat.py @@ -109,8 +109,13 @@ def fixup_c(filename): directive = m.group(2) if directive in ("if ", "ifdef ", "ifndef "): l_next = lines[0] + label = re.match(f"^ +[a-z_]+:", l_next) indent_next = len(re.match(r"( *)", l_next).group(1)) - if indent - 4 == indent_next and re.match(r" +(} else |case )", l_next): + if label: + l = l[indent:] + lines[0] = lines[0].lstrip() + dedent_stack.append(0) + elif indent - 4 == indent_next and re.match(r" +(} else |case )", l_next): # This #-line (and all associated ones) needs dedenting by 4 spaces. l = l[4:] dedent_stack.append(indent - 4)

What about the slightly more verbose:

#if MICROPY_REPL_EMACS_EXTRA_WORDS_MOVE backward_word: #endif #if MICROPY_REPL_EMACS_WORDS_MOVE case 'b': // rest goes here #endif

... to avoid nesting multiple #ifdefs in these?

EDIT: This doesn't work without another ifdef after it

Or, as forward_word and backward_word are labels, maybe they could always be included but marked as unused to avoid the warning (not sure how to do that for labels, though...)

I don't know what level of crime this is, but it seems to work and doesn't rely on any compiler-specific pragmas:

#if MICROPY_REPL_EMACS_WORDS_MOVE case 'b': goto backward_word; // Ensure label always used backward_word: redraw_step_back = cursor_count_word(0); rl.escape_seq = ESEQ_NONE; break; case 'f': goto forward_word; // Ensure label is always used forward_word: redraw_step_forward = cursor_count_word(1); rl.escape_seq = ESEQ_NONE; break; case 'd': vstr_cut_out_bytes(rl.line, rl.cursor_pos, cursor_count_word(1)); redraw_from_cursor = true; rl.escape_seq = ESEQ_NONE; break; case 127: goto backward_kill_word; // Ensure label is always used backward_kill_word: redraw_step_back = cursor_count_word(0); vstr_cut_out_bytes(rl.line, rl.cursor_pos - redraw_step_back, redraw_step_back); redraw_from_cursor = true; rl.escape_seq = ESEQ_NONE; break; #endif

Gadgetoid · 2024-06-05T14:26:46Z

I should probably rebase this 😬

dpgeorge · 2024-06-05T23:33:30Z

I should probably rebase this 😬

Yes please!

Gadgetoid · 2024-06-06T22:25:04Z

Hmm.

This seems to have dramatically increased in scope since last I ran ci_c_code_formatting_run
My computer and CI disagree about code formatting
My local version of Uncrustify (Homebrew on a Mac) is 0.79.0_f
CI is running 0.72.0_f

Trying again from my Pi which has 0.72.0.

Looks like rules can change quite dramatically between versions, so we should look into some way of enforcing either the specific Uncrustify version, or the rules from a specific version- otherwise well-meaning contributors could run the code formatting run and submit code that fails CI... as I just did.

If you want a version that matches what's expected by the current (actually pretty arbitrary) version used in CI, run:

wget https://github.com/uncrustify/uncrustify/archive/refs/tags/uncrustify-0.72.0.tar.gz
tar -xf uncrustify-0.72.0.tar.gz
cmake -B build -S uncrustify-uncrustify-0.72.0
cmake --build build -j
build/uncrustify --version

The above should probably replace ci_c_code_formatting_setup since it works on my mac (happy bonus) and Pi.

eg:

diff --git a/.gitignore b/.gitignore
index 2d20cb189..483ce0939 100644
--- a/.gitignore
+++ b/.gitignore
@@ -23,3 +23,6 @@ user.props
 
 # MacOS desktop metadata files
 .DS_Store
+
+# CI Tools
+tools/uncrustify
diff --git a/tools/ci.sh b/tools/ci.sh
index b0746178b..19d0c116a 100755
--- a/tools/ci.sh
+++ b/tools/ci.sh
@@ -21,8 +21,11 @@ function ci_gcc_arm_setup {
 # c code formatting
 
 function ci_c_code_formatting_setup {
-    sudo apt-get install uncrustify
-    uncrustify --version
+  wget https://github.com/uncrustify/uncrustify/archive/refs/tags/uncrustify-0.72.0.tar.gz
+  tar -xf uncrustify-0.72.0.tar.gz
+  cmake -B tools/uncrustify -S uncrustify-uncrustify-0.72.0
+  cmake --build tools/uncrustify -j
+  ./tools/uncrustify/uncrustify --version
 }
 
 function ci_c_code_formatting_run {
diff --git a/tools/codeformat.py b/tools/codeformat.py
index 30763118f..98e775cc3 100755
--- a/tools/codeformat.py
+++ b/tools/codeformat.py
@@ -175,7 +175,7 @@ def main():
 
     # Format C files with uncrustify.
     if format_c:
-        command = ["uncrustify", "-c", UNCRUSTIFY_CFG, "-lC", "--no-backup"]
+        command = ["./tools/uncrustify/uncrustify", "-c", UNCRUSTIFY_CFG, "-lC", "--no-backup"]
         if not args.v:
             command.append("-q")
         batch(command)

Update: raised a separate PR for this here: #15221

projectgus · 2024-06-11T00:41:48Z

Looks like rules can change quite dramatically between versions, so we should look into some way of enforcing either the specific Uncrustify version, or the rules from a specific version- otherwise well-meaning contributors could run the code formatting run and submit code that fails CI... as I just did.

Yeah, it's not ideal and can be generally frustrating. I put a small note in the docs when I ran across the same thing, but I think most contributors won't expect it.

I did a bit of work earlier this year building uncrustify for WASM, and had it running under wasmtime. This creates the interesting possibility of publishing uncrustify packages on pypi for easy install, and switching the pre-commit hook to use the correct uncrustify version under WASM by default. I stopped working on it, but maybe I should pick it back up.

(Performance is... OK. The overhead is mostly startup time unfortunately, so it does add a perceptible delay when running pre-commit against small patches. It's probably not enough of a delay to make it infeasible, though, and would always include an option to use native uncrustify instead.)

dlech · 2024-06-11T01:02:30Z

publishing uncrustify packages on pypi for easy install

It's not WASM, but it is already there. https://pypi.org/project/micropython-uncrustify/ The binary wheels should cover the vast majority of developer machines and it has a proper source package if anyone want to build it somewhere exotic.

Happy to transfer the project to MicroPython maintainership and save you the work.

projectgus · 2024-06-11T01:34:19Z

@dlech Ah sorry I'm crossing back and forth the various discussion streams about this. Have just left some related replies at #15221 (comment) and https://github.com/orgs/micropython/discussions/14496#discussioncomment-9733050

Add the following new PATHS: * drivers/**/*.[ch] * examples/**/*.[ch] * shared/libc/*.[ch] * shared/readline/*.[ch] Add an exclusion for "drivers/cc3100/*/*.[ch]". Update "extmod/nimble" path to include "**". Signed-off-by: Phil Howard <phil@gadgetoid.com>

Gadgetoid · 2024-10-31T10:39:33Z

I'm still missing the proposed changes to shared/readline/readline.c but I've rebased this to catch up with the last checks notes four months of updates.

Avoid getting fancy with MICROPY_MALLOC_USES_ALLOCATED_SIZE and make the function calls with/without this define explicit and readable. Signed-off-by: Phil Howard <github@gadgetoid.com>

Rewrite the MICROPY_REPL_EMACS_WORDS_MOVE section to avoid #if/#endif being indented. codeformat.py runs some fixups to post-process uncrustify's formatting output. Crucially these try to unindent preprocesor macros for better readability, but do not work in this instance. Co-authored-by: Phil Howard <github@gadgetoid.com> Signed-off-by: Phil Howard <github@gadgetoid.com>

Run code formatting to apply rules to: * drivers/**/*.[ch] * examples/**/*.[ch] * shared/libc/*.[ch] * shared/readline/*.[ch] Signed-off-by: Phil Howard <phil@gadgetoid.com>

Gadgetoid · 2024-10-31T11:23:13Z

shared/readline/readline.c

-#endif
+                #if MICROPY_REPL_EMACS_KEYS
+            up_arrow_key:
+                #endif


I'm tempted to skip changes to readline.c in favour of making a separate PR to clean up these warts. It feels a little out of scope to apply a manual rewrite here?

@Gadgetoid That seems like a very good idea to me. If you're going to make that change then feel free to request a review after that and I'll take another pass over it. Thanks for perservering!

Gadgetoid mentioned this pull request Mar 6, 2024

global: Prune trailing whitespace. #13777

Closed

Gadgetoid force-pushed the patch-uncrustify branch from f35f941 to 2045185 Compare March 8, 2024 12:40

Gadgetoid commented Mar 8, 2024

View reviewed changes

Gadgetoid force-pushed the patch-uncrustify branch 2 times, most recently from 66cd953 to e45b202 Compare June 6, 2024 22:23

Gadgetoid mentioned this pull request Jun 6, 2024

tools: Ensure codeformat.py uses uncrustify 0.72. #15221

Open

Gadgetoid force-pushed the patch-uncrustify branch 2 times, most recently from 0001965 to b71e692 Compare October 31, 2024 10:04

extmod/nimble: Better code readability.

ea9c8e4

Avoid getting fancy with MICROPY_MALLOC_USES_ALLOCATED_SIZE and make the function calls with/without this define explicit and readable. Signed-off-by: Phil Howard <github@gadgetoid.com>

Gadgetoid force-pushed the patch-uncrustify branch from b71e692 to 0c1298a Compare October 31, 2024 11:03

projectgus and others added 2 commits October 31, 2024 11:04

all: Apply code formatting to new paths.

17e6da0

Run code formatting to apply rules to: * drivers/**/*.[ch] * examples/**/*.[ch] * shared/libc/*.[ch] * shared/readline/*.[ch] Signed-off-by: Phil Howard <phil@gadgetoid.com>

Gadgetoid force-pushed the patch-uncrustify branch from 0c1298a to 17e6da0 Compare October 31, 2024 11:05

Gadgetoid commented Oct 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

global: Run uncrustify on all .c source files. #14046

global: Run uncrustify on all .c source files. #14046

Gadgetoid commented Mar 6, 2024 •

edited

Loading

codecov bot commented Mar 6, 2024 •

edited

Loading

github-actions bot commented Mar 6, 2024 •

edited

Loading

jimmo commented Mar 7, 2024

projectgus commented Mar 7, 2024

Gadgetoid commented Mar 8, 2024

Gadgetoid Mar 8, 2024

Gadgetoid Mar 8, 2024

Gadgetoid Mar 8, 2024

Gadgetoid Mar 8, 2024

projectgus Mar 12, 2024 •

edited

Loading

projectgus Mar 12, 2024

projectgus Mar 13, 2024 •

edited

Loading

Gadgetoid commented Jun 5, 2024

dpgeorge commented Jun 5, 2024

Gadgetoid commented Jun 6, 2024 •

edited

Loading

projectgus commented Jun 11, 2024 •

edited

Loading

dlech commented Jun 11, 2024

projectgus commented Jun 11, 2024

Gadgetoid commented Oct 31, 2024

Gadgetoid Oct 31, 2024

Gadgetoid Oct 31, 2024

projectgus Nov 1, 2024

global: Run uncrustify on all .c source files. #14046

Are you sure you want to change the base?

global: Run uncrustify on all .c source files. #14046

Conversation

Gadgetoid commented Mar 6, 2024 • edited Loading

codecov bot commented Mar 6, 2024 • edited Loading

Codecov Report

github-actions bot commented Mar 6, 2024 • edited Loading

jimmo commented Mar 7, 2024

projectgus commented Mar 7, 2024

Gadgetoid commented Mar 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

projectgus Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

projectgus Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Gadgetoid commented Jun 5, 2024

dpgeorge commented Jun 5, 2024

Gadgetoid commented Jun 6, 2024 • edited Loading

projectgus commented Jun 11, 2024 • edited Loading

dlech commented Jun 11, 2024

projectgus commented Jun 11, 2024

Gadgetoid commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gadgetoid commented Mar 6, 2024 •

edited

Loading

codecov bot commented Mar 6, 2024 •

edited

Loading

github-actions bot commented Mar 6, 2024 •

edited

Loading

projectgus Mar 12, 2024 •

edited

Loading

projectgus Mar 13, 2024 •

edited

Loading

Gadgetoid commented Jun 6, 2024 •

edited

Loading

projectgus commented Jun 11, 2024 •

edited

Loading