clang-format implementation

[ia]

• Please check if the PR fulfills these requirements

• The changes have been tested locally
• There are no breaking changes¹

• What kind of change does this PR introduce?
  clang-format check on github CI.

• What is the current behavior?
  make check-style is not running on github CI via actions. It successful only because it doesn't return error code to the environment: find external tool is not provided in a container but busybox built-in implementation of find doesn't support required flag for find in check-style target.

• What is the new behavior (if this is a feature change)?
  Make check-style run "successfully"¹ (unfortunately) with a lot of warnings.

• Other information:
  Maybe I miss something but while I was working on a slightly different modifications, nearly by accident I discover this situation. Is there any chance that you're aware of that and this is a supposed behavior?

---

P.S.

To be fully honest with you, I personally in my humble opinion find some of the suggestions by clang-format a bit strange because for me they look like "formality over read-ability" like enum style declarations or some recommendations on comments style. So maybe instead of making clang-format happy, just .clang-format config can be tweaked a bit. ;)

Plus, some source files, especially those which were auto-generated by STM-related IDEs or obtained as 3rd party projects as FreeRTOS/usb-pd/etc., probably should be excluded at all from checking.

Regardless your decision over formatting rules I will be glad to help with code reformatting. As always, let me know what you think about all of this.

Oh, and I took some liberty to split/rename build steps with tests & checks for unification in naming and splitting by code language.

Footnotes

1. it depends how to treat it. ↩ ↩²

[ia]

Oh, I just noticed that this is not a strictly required build step. But still, having at least one red light is not quite good, I guess. 🤔

[Ralim]

I'm happy to exclude freertos and driver folders from clang format.

The re isn't much in this format I insist on; mostly I use this to stop my ide's folding code at 80 characters or putting {} on their own lines.

[ia]

Ookay... I tweaked a bit ALL_INCLUDES & ALL_SOURCE by excluding everything related to ST/CMSIS/HAL/etc. And I must admit that some suggestions looks pretty neat & tidy. But for some warnings it thinks that it knows better. :D

I tried briefly to find some options myself... but I got luck with enums only. So, what I would disable in the first place:

• how comments must be aligned
• how macros must be aligned
• how to align spaces in complex conditions

But .clang-format config seems pretty sophisticated so I guess the main approach is to keep googling & trying stackoverflow advises sit down and read very carefully official documentation considering that some "high level" options may enable/disable another set of "low level" ones (as far as I could understand from brief docs look through).

[Ralim]

What we may want to do is instead pick an upstream standard one clang-format knows (Google,LLVM etc) and then just stack overrides on that.
I often default to LLVM iirc in IDE's where there isnt one setup yet.

[ia]

Official documentation doesn't make much of a sense to me for some reason BTW. For example, it says:

AlignConsecutiveMacros (AlignConsecutiveStyle) **clang-format 9**
    Style of aligning consecutive macro definitions.
[...]
AlignConsecutiveMacros: AcrossEmptyLines

AlignConsecutiveMacros:
  Enabled: true
  AcrossEmptyLines: true
  AcrossComments: false

Okay, sounds what can be tweaked to disable warnings for macros. Check version in docker:

$ clang-format --version
Alpine clang-format version 13.0.1

Okay, 13 > 9. Trying by adding this into .clang-format:

AlignConsecutiveMacros:
  Enabled: true
  AcrossEmptyLines: false
  AcrossComments: false

and run check-style to get multiple errors like:

/build/source/source/./.clang-format:11:3: error: unknown enumerated scalar
  Enabled: true
  ^
Error reading /build/source/source/./.clang-format: Invalid argument

and even after removing Enabled: true line I get:

/build/source/source/./.clang-format:11:3: error: unknown enumerated scalar
  AcrossEmptyLines: false
  ^

😶

[ia]

What we may want to do is instead pick an upstream standard one clang-format knows (Google,LLVM etc) and then just stack overrides on that. I often default to LLVM iirc in IDE's where there isnt one setup yet.

As far as I understand, the current .clang-format has been generated based on LLVM code style. However, my another understanding is that the way how clang-format works is pure binary logic for every option: true to mess code according to an option, false to mess code against to an option. But there is no "don't touch" option (except putting special comment into a file but then the whole file will be ignored): even when I tweak some true/false options (or just commenting them with #) locally, clang-format suggests me to change the code in one way or another but not keeping it as is for some reason. Probably I'm just not getting something. Anyway, sorry to waste your time on this not so much important thing, really.

[ia]

Ufff... what an adventure! Here is a brief write-up of what has been done:

• out of curiosity I decided to see what I could do;
• but to navigate over suggestions from clang-format with comfort I needed a proper log format;
• to do that, I had to update Makefile;
• but since I had to work with Makefile anyway, I decided to unify its style first too;
• then I updated check-style target - now it should show gcc-like error line pointing out to file:line:column which helps a lot in the environments where this format is used by default to navigate through errors in source code;
• and then little by little I was making suggested formatting changes.

Surprisingly, there were not so many big issues and considering that the rest of the files are having coherent style, I guess current tweaks of clang-format config relatively reasonable. Speaking of which, what I had to trade-off with:

• I disabled option to tell to make include in alphabet order (PinecilV2 / BLE stuff complexity of declarations in particular order from headers);
• I used some suggestions for heap_5.c but then I excluded it since it's very hardware-specific piece as far as I understand;
• and after some jumps around I disabled clang-format only for section of menuitems inside settingsGUI.cpp (once again, only due to very specific nature of this part of code).

Now, as of for these two "expected checks"... as far as I understand from this conversation, since there is an update of workflow/push.yml which is related to origin repo/branch management, github doesn't allow (even as a part of Pull-Request) to mess with jobs/build steps. So I guess you should manually to commit changes into workflow/push.yml so everything would turn into green for merge here. And yes, in my humble opinion I would like to see test steps separated & renamed a bit but only if you don't mind. As always, will be waiting your reaction.

---

P.S.

I swear, I came to this project only to do a couple of PRs for tiny features for my own single one iron... and I even didn't have time to start work on them yet! 🤣

[ia]

And here is how build steps look with current push.yml.

[ia]

Brief status update:

• the more I worked with clang-format the more I see the usefulness of it to a certain degree;
• since 99%¹ of IronOS code formatted according to current .clang-format config, I decided to make it pass the checks and even kept SortIncludes option;
• there is also very useful option InsertBraces for clang-format but it supported only since version 15 while reference version of Alpine Linux for docker has 13;
• but I did a bit of some sed/grep syntax madness to implement the imitation of this option since I work on this anyway;
• I'm sorry if it's getting too far especially with "manual" unification of some parts of code but you always can tell me what to fix/undo;
• I understand that code review for this merge could be complicated but we can discuss any concerns (my own is that I hope I didn't miss/put extra bracket or something which can be detected only in the field);
• but in general currently I personally consider this task as complete.

Please, let me know what you think because I can't wait to hear back from you & crew.

P.S. If I have to rationale the braces for loop/conditions then I can say that in the most inappropriate moment it can easily fire back when you in the heat of testing/debugging/fixing put just only one extra line to some block not having them.

Footnotes

1. 100% in this branch ↩

[ia]

I have a couple of thoughts how to improve a bit manageability of check-style target in Makefile but I would like to make it as a separate pull request (in case if this one will be accepted) since this one is getting pretty bloated already.

[Ralim]

Yeah definitely seperate PR.

I think this is good to go btw, just want to recheck when I'm more awake 😂