Clang Formatting

[wyattearp]

So I was going to put this in the chat, but then it became a lot more data than I figured was it was intended for.

TL;DR - looks like Mozilla might be the closest default if you wanted to pick a place to start clang-formatting the code, followed by Microsoft.

I started with the ui/ui_term.c file as my basis since it I was familiar with how I figured it should look and applied each of the clang formatting passes. Just for grins, here’s how that looked:

Style	%diff
LLVM	71
Google	91
Chromium	87
Mozilla	87
Webkit	71
Microsoft	64
GNU	84

Oddly, the Microsoft one was the “closest” just based on changes, which I wouldn’t have guess. but it does tend to allow the code to be wider it seems. My poking at it manually, it looks like Mozilla or Webkit is closer but there’s some difference as there always will be. So I looked at it more and then I did the most fun thing I could do – I pasted a couple of function into chatGPT and it suggested Mozilla.

Personally, I think either Webkit or Microsoft would do well with probably needing to move to 4 spaces instead of 2 and then it would likely match a little better. I’ll paste a couple of code samples in a few from the ui_term.c so you can see the difference.

[wyattearp]

Original

void ui_term_progress_bar(uint32_t current, uint32_t total)
{
    uint32_t pct = (current*20)/(total);
    printf("\r%s[", ui_term_color_prompt());
    for(int8_t i=0; i<20; i++)
    {
        if(pct<i) if(i%2) printf(" "); else printf("o");
        else if(pct==i) printf("%sc", ui_term_color_notice());
        else if(pct>i) printf("-");
    }
    printf("%s]\r\e[1C",ui_term_color_prompt());
}

void ui_term_progress_bar_draw(ui_term_progress_bar_t *pb)
{
    system_config.terminal_hide_cursor=true;
    busy_wait_ms(1);
    printf("%s\r%s[%s", ui_term_cursor_hide(), ui_term_color_prompt(), ui_term_color_reset());
    for(int8_t i=0; i<20; i++)
    {
        if(i%2) printf(" "); else printf("o");
    }
    printf("%s]\r\e[1C",ui_term_color_prompt());
    pb->indicator_state=1;
    pb->previous_pct=0;
    pb->progress_cnt=0;
}

[wyattearp]

Webkit

void ui_term_progress_bar(uint32_t current, uint32_t total)
{
    uint32_t pct = (current * 20) / (total);
    printf("\r%s[", ui_term_color_prompt());
    for (int8_t i = 0; i < 20; i++) {
        if (pct < i)
            if (i % 2)
                printf(" ");
            else
                printf("o");
        else if (pct == i)
            printf("%sc", ui_term_color_notice());
        else if (pct > i)
            printf("-");
    }
    printf("%s]\r\e[1C", ui_term_color_prompt());
}

void ui_term_progress_bar_draw(ui_term_progress_bar_t* pb)
{
    system_config.terminal_hide_cursor = true;
    busy_wait_ms(1);
    printf("%s\r%s[%s", ui_term_cursor_hide(), ui_term_color_prompt(), ui_term_color_reset());
    for (int8_t i = 0; i < 20; i++) {
        if (i % 2)
            printf(" ");
        else
            printf("o");
    }
    printf("%s]\r\e[1C", ui_term_color_prompt());
    pb->indicator_state = 1;
    pb->previous_pct = 0;
    pb->progress_cnt = 0;
}

[wyattearp]

Mozilla (note the extra new lines from the type of the function causing the difference in the git diff):

void
ui_term_progress_bar(uint32_t current, uint32_t total)
{
  uint32_t pct = (current * 20) / (total);
  printf("\r%s[", ui_term_color_prompt());
  for (int8_t i = 0; i < 20; i++) {
    if (pct < i)
      if (i % 2)
        printf(" ");
      else
        printf("o");
    else if (pct == i)
      printf("%sc", ui_term_color_notice());
    else if (pct > i)
      printf("-");
  }
  printf("%s]\r\e[1C", ui_term_color_prompt());
}

void
ui_term_progress_bar_draw(ui_term_progress_bar_t* pb)
{
  system_config.terminal_hide_cursor = true;
  busy_wait_ms(1);
  printf("%s\r%s[%s",
         ui_term_cursor_hide(),
         ui_term_color_prompt(),
         ui_term_color_reset());
  for (int8_t i = 0; i < 20; i++) {
    if (i % 2)
      printf(" ");
    else
      printf("o");
  }
  printf("%s]\r\e[1C", ui_term_color_prompt());
  pb->indicator_state = 1;
  pb->previous_pct = 0;
  pb->progress_cnt = 0;
}

[wyattearp]

Microsoft

void ui_term_progress_bar(uint32_t current, uint32_t total)
{
    uint32_t pct = (current * 20) / (total);
    printf("\r%s[", ui_term_color_prompt());
    for (int8_t i = 0; i < 20; i++)
    {
        if (pct < i)
            if (i % 2)
                printf(" ");
            else
                printf("o");
        else if (pct == i)
            printf("%sc", ui_term_color_notice());
        else if (pct > i)
            printf("-");
    }
    printf("%s]\r\e[1C", ui_term_color_prompt());
}

void ui_term_progress_bar_draw(ui_term_progress_bar_t *pb)
{
    system_config.terminal_hide_cursor = true;
    busy_wait_ms(1);
    printf("%s\r%s[%s", ui_term_cursor_hide(), ui_term_color_prompt(), ui_term_color_reset());
    for (int8_t i = 0; i < 20; i++)
    {
        if (i % 2)
            printf(" ");
        else
            printf("o");
    }
    printf("%s]\r\e[1C", ui_term_color_prompt());
    pb->indicator_state = 1;
    pb->previous_pct = 0;
    pb->progress_cnt = 0;
}

[ian]

Nice! That’s really interesting.

The code base is 16 years+ old and has been worked on by tons of people. My code style is like my mandarin accent - all over the place. Feel free to override whats there, I’ll happily code to a standard especially if there is formatting help/enforcement.

All of these have hanging brackets after the function declaration. I used to do that, but have been trying to clean it up as I go after reading through Paul’s very professional C code in scope.c.

void ui_term_progress_bar_draw(ui_term_progress_bar_t *pb)
{

Becomes

void ui_term_progress_bar_draw(ui_term_progress_bar_t *pb){

However Paul is full on and does this:

void 
ui_term_progress_bar_draw(ui_term_progress_bar_t *pb){

and I’m not yet a fan of that.

As I rework code I’ve been removing brackets from their own line to make the code a bit more terse, with less stuff to scroll through to find what I need. Not stating a preference, just flagging what I’ve been doing in the /pirate/ library and /commands/ code files.

[grymoire]

@ian

However Paul is full on and does this:

void 
ui_term_progress_bar_draw(ui_term_progress_bar_t *pb){

and I’m not yet a fan of that.

I’m an old fashioned coder. One thing I don’t like about this is that if I grep a codebase for a function name, it doesn’t return the function type.

[wyattearp]

I’m also in the camp of “I’ll code to whatever is asked”

I’d agree as well - grep is my spirit animal. I’ve also noticed that each of the above permit if statements without {} … which just always freaks me out because of how many errors I’ve run across in code. Internally, we’ve always said it had to fit on one line if you were doing that, but just don’t do that and we’ll all call it good

[AreYouLoco]

I recommend you guys addon for VSCodium called “Indent Rainbow” makes every indent level different color and its super easy to read like that

[wyattearp]

An interesting plugin for sure. I think the goal is that the code is consistent with whatever editor your using.

[ian]

I am also terrified of if thens on multiple lines without brackets. I get it may be faster to write, but also you don’t get hints like color coded brackets to help figure out what level you’re at if something goes wrong.

These days I use copilot, which is absolutely useless a lot of the time, but it does scaffold if then with brackets and I can tweak as needed. The fancy autocomplete is handling a lot of that grunt so I don’t see the point in leaving it out.

[ian]

Just curious what the next step would be for using clang? It seems like something that would be beneficial, but I’m not familiar with it yet.

[henrygab]

@ian

I am also terrified of if thens on multiple lines without brackets

As you should be. Some of the most gnarly bugs seem to come from omitting braces and macro expansion.

You should also know that clang-format (and others) can force if/else to have braces.

As for macros, as a coding standard, I would recommend:

• enforcing that all macros are single-statement. This may require a do { ... } while (0) construct if the macro contains multiple statements.
• careful code review to ensure single-evaluation of macro parameters. Carefully crafting the macro to use a temporary (which the optimizer can then remove) is sometimes needed.

For CLang formatting, I am willing to generate a starting options file. Then, folks who care should apply it, and suggest any changes (with rationale for why to change it).

After agreement is reached on the format options:

1. Ask folks if there are any significant outstanding work … and whether they are OK rebasing (because merging is very difficult, so better for folks to rebase before they generate a PR).

2. Create a branch for the reformat.

3. A single commit should cover the clang-format results (with the agreed upon options file).

4. Verify things all work, then merge into main branch.

[henrygab]

Obviously, the clang-format options file should be added to the repository also.

Optionally, I believe there are github actions that will reject a pull request that does not conform to the formatting. Helps to keep the main branch clean and all that…

[wyattearp]

Soooo… About #3… I wish that was accurate however it’s resulted in multiple compilation failures, mostly due to include orders.

I’ll take the feedback here and punch of a file for a draft review on the test branch in a bit. I haven’t abandoned this, we’ve just had a busy week with family traveling so I’ve not had any time to commit to this.

[ian]

No rush, I just want to make sure I haven’t neglected to do something

[henrygab]

@wyattearp

Soooo… About #3… I wish that was accurate however it’s resulted in multiple compilation failures, mostly due to include orders.

There is a simple fix for that… setup clang-format to not mess with the include ordering. Really … it’s not a “safe” option, and options which break valid code just to alphabetize includes is not worth it.

Based on your message, I am happy to defer to you to manage this update.

[ian]

Yeah there’s a few widely used structs that get dragged around way too much, so the include order ends up being super important. Things like the response struct for command processing, prompt and menu formatting too.

I did get rid of a few nasty ones, like the opt_args nonsense. /Pirate/ library also tamed it a bit, so you don’t need eleventy includes just to get the ADC to read a voltage

[wyattearp]

Alright, life chaos contained for the moment. I thought about putting this up in a PR, but it’s pretty noisy. Based on initial discussions, try the following with clang-format -i <file> after placing it in the top level directory:

BasedOnStyle: Mozilla
IndentWidth: 4
TabWidth: 4
UseTab: Never
ContinuationIndentWidth: 4
ConstructorInitializerIndentWidth: 4
AllowAllArgumentsOnNextLine: true
AllowAllParametersOfDeclarationOnNextLine: true
NamespaceIndentation: All
SortIncludes: false
InsertBraces: true
BreakBeforeBraces: Attach
ColumnLimit: 120
UseCRLF: true

One note is that I moved CRLF to true - this is more because there exists 75415 CRLF endings as opposed to the 12884 LF endings in the code base … and would thus be a lower impact (but I like Unix better ).

Give it a shot and let me know. I’d suggest the following files for experimentation:

• pirate.c
• mode/hwi2.c

Let me know what you think

[ian]

Nice! Thank you.

I followed the instructions here to get it going in VScode:

• Install the Clang-Format extension by Xaver Hellauer
• Install clang
• Put the formatting rules in .clang-format in the root project directory
• Edit clang-format extension settings - in assume filename add .clang-format
• Restart VSCode
• ALT + SHIFT + F to format the currently active file on windows. (CTRL + SHIFT + I on linux)

You may need to add the install folder to your system environment path: %LLVM% \bin

Evidently I followed these instructions some time ago, but didn’t have the .clang-format file and dead-ended.

I believe this is working If I delete the .clang-format file I get different formatting, so probably ok?

I’m going to see if I can tweak the struct formatting a bit, this seems a bit “robust”. The proper solution is probably to tame the menu and prompt structs, but that’s a medium term project.

The function type on it’s own line is a bit jarring to me. This reddit thread is enlightening. It seems to be a hold over from using macros to bridge K&R and C89 compatibility. It seems like advanced coders (at least the old school type) prefer it, especially for working on emacs style editors.

AlwaysBreakAfterReturnType: None
AlwaysBreakAfterDefinitionReturnType: None

Just to get familiar with clang-format, adding these two lines keep the function and return type on the same line.

When we’re happy with the formatting, I’d vote for moving to LF only in a big update, and then enforcing it in the future. There is (was?) an issue with the PIO assembler under an OS (mac?) failing due to CRLF. Probably there is a way to do automated formatting of pull requests on github, or I can knock something together on the build server.