Skip to content

Uncrustify: Update and include new config options #522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Jun 24, 2025

This is another round of trying to use uncrustify to make our code consistent and preferably similar in structure to what rustfmt does. Some features are new so this updates uncrustify. That caused some changes as uncrustify fixed some bugs.

Each option/change is its own commit.

New changes:

  • Force a newline at the end of files
  • Use unix line endings
  • Align "" character in macros
  • Force 1 or 2 blank lines between functions
  • Remove newlines at the end of a function
  • Sort #includes. This works pretty well!

@lschuermann
Copy link
Member

@bradjc I tacked on an update to the uncrustify dependency in shell.nix.

@lschuermann
Copy link
Member

lschuermann commented Jun 25, 2025

  • Sort #includes. This works pretty well!

This breaks one of the u8g2 demo apps:

leons@iron ~/p/t/l/e/t/u/led-menu (uncrustify-config-2025-06)> git diff master -- main.c
diff --git a/examples/tests/u8g2-demos/led-menu/main.c b/examples/tests/u8g2-demos/led-menu/main.c
index 1a498719..ccd3f1c3 100644
--- a/examples/tests/u8g2-demos/led-menu/main.c
+++ b/examples/tests/u8g2-demos/led-menu/main.c
@@ -6,11 +6,10 @@
 #include <libtock/interface/led.h>
 
 // These have to be included before mui.h
-#include <u8g2-tock.h>
-#include <u8g2.h>
-
 #include <mui.h>
 #include <mui_u8g2.h>
+#include <u8g2-tock.h>
+#include <u8g2.h>
 
 u8g2_t u8g2;
 mui_t ui;
leons@iron ~/p/t/l/e/t/u/led-menu (uncrustify-config-2025-06)> make
  CC        main.c
In file included from main.c:10:
../../../../u8g2/u8g2-c4f9cd9f8717661c46be16bfbcb0017d785db3c1/csrc/mui_u8g2.h:165:1: error: unknown type name 'u8g2_uint_t'
  165 | u8g2_uint_t mui_get_x(mui_t *ui);
      | ^~~~~~~~~~~
../../../../u8g2/u8g2-c4f9cd9f8717661c46be16bfbcb0017d785db3c1/csrc/mui_u8g2.h:166:1: error: unknown type name 'u8g2_uint_t'
  166 | u8g2_uint_t mui_get_y(mui_t *ui);
      | ^~~~~~~~~~~
../../../../u8g2/u8g2-c4f9cd9f8717661c46be16bfbcb0017d785db3c1/csrc/mui_u8g2.h:167:1: error: unknown type name 'u8g2_t'
  167 | u8g2_t *mui_get_U8g2(mui_t *ui);
      | ^~~~~~
../../../../u8g2/u8g2-c4f9cd9f8717661c46be16bfbcb0017d785db3c1/csrc/mui_u8g2.h:169:42: error: unknown type name 'u8g2_uint_t'  169 | void mui_u8g2_draw_button_utf(mui_t *ui, u8g2_uint_t flags, u8g2_uint_t width, u8g2_uint_t padding_h, u8g2_uint_t padding_v, const char *text);
      |                                          ^~~~~~~~~~~
[...]

From the uncrustify source:

# If TRUE, will sort consecutive single-line '#include' statements [C/C++] and '#import' statements [Obj-C]
# This is generally a bad idea, as it may break your code.
mod_sort_include                         = false    # false/true

I do think it's potentially dangerous, albeit nice if it works. Can we disable it on a case-by-case basis, e.g., through comments?

@brghena
Copy link
Contributor

brghena commented Jun 25, 2025

Ugh. Sorting includes shouldn't matter. But given that external libraries are of varying quality, I think we should probably disable include sorting.

Although I think it would still be reasonable to sort them all once, leave the ones that compile fine alone, fix the ones that don't compile, and then disable the uncrustify option again. So they would be mostly sorted now, just not maintained.

@bradjc
Copy link
Contributor Author

bradjc commented Jun 25, 2025

I actually submitted a patch to fix that: olikraus/u8g2@f36cc20

@ppannuto
Copy link
Member

Hmm..., I still have modest mixed feelings about the include-sorting. One of the things that our examples currently do (very intentionally) is sort includes into logical "blocks" of where things came from, e.g.,
image

has a block of libc things, then a block of nrf ble library things, then a block of libtock things.

After this, the libc and nrf things end up mixed up :/. Now, libtock hangs out on its own, because we intentionally set an expectation of pointing the compiler include path below the libtock/**.h things (which allows uncrustify regex rules to work). But I don't think we can do the same for something like #include <nrf/eddystone.h> since any includes inside eddystone would break.

I wish there was a way to force uncrustify to format blocks of includes separately... can't find that though :/

@bradjc
Copy link
Contributor Author

bradjc commented Jun 25, 2025

Personally I think the upside is high, compared to ble-uart which I doubt anyone has run in 3+ years. Who has hail/imix any more?

@ppannuto
Copy link
Member

... I had been thinking about creating a tock/libtock-c-archive or similar [or maybe it can just be a folder of tock/tock-archive...]. Some unimportant crap in serialization hung me up for a few hours with the C23 stuff and I was cursing the old/unused code.

We can merge this after (at the end of?) the tutorial Friday?

@bradjc
Copy link
Contributor Author

bradjc commented Jun 25, 2025

We can merge this after (at the end of?) the tutorial Friday?

Unfortunately fixing the ordering requirement for u8g2 is going to be a bit of thing. Apologies in advance. PR coming. Sidenote, if cmake actually can do everything for us...that would be amazing. But, getting this all to "just works" is far from trivial.

@ppannuto
Copy link
Member

Ah, thought that merged upstream PR meant it was a non-issue already — guessing some other updates to the library are bringing unhappiness when bumping that submodule?

@lschuermann
Copy link
Member

After this, the libc and nrf things end up mixed up :/. Now, libtock hangs out on its own, because we intentionally set an expectation of pointing the compiler include path below the libtock/**.h things (which allows uncrustify regex rules to work). But I don't think we can do the same for something like #include <nrf/eddystone.h> since any includes inside eddystone would break.

@ppannuto I do agree with you on that, and I think this example is sufficient to sway me to say I'd rather not see include sorting be mandatory. At the very least, we should ensure that any stdlib includes come before other library includes (which, I guess, would mean exhaustively listing out stdlib header files in the uncrustify.cfg...).

@bradjc
Copy link
Contributor Author

bradjc commented Jun 25, 2025

After this, the libc and nrf things end up mixed up :/. Now, libtock hangs out on its own, because we intentionally set an expectation of pointing the compiler include path below the libtock/**.h things (which allows uncrustify regex rules to work). But I don't think we can do the same for something like #include <nrf/eddystone.h> since any includes inside eddystone would break.

@ppannuto I do agree with you on that, and I think this example is sufficient to sway me to say I'd rather not see include sorting be mandatory. At the very least, we should ensure that any stdlib includes come before other library includes (which, I guess, would mean exhaustively listing out stdlib header files in the uncrustify.cfg...).

I guess it's a tradeoff between allowing very carefully addressed cases and bringing everything up to a general standard. I prefer to have the floor. For example I think having this consistency makes more of a difference:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants