Clang-Format the FreeRTOS-Kernel

[Skptak]

Description

Swap all files to use clang-format instead of uncrustify
Use a new spell checker
Use new version of some CI-CD Actions
Relevant PR for CI-CD-Actions can be found here

Test Steps

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1c465a0) 93.64% compared to head (11cafda) 93.74%.
Report is 1 commits behind head on main.

❗ Current head 11cafda differs from pull request most recent head 804a685. Consider uploading reports for the commit 804a685 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #762      +/-   ##
==========================================
+ Coverage   93.64%   93.74%   +0.09%     
==========================================
  Files           6        6              
  Lines        2549     2589      +40     
  Branches      608      610       +2     
==========================================
+ Hits         2387     2427      +40     
  Misses        107      107              
  Partials       55       55              

Flag	Coverage Δ
unittests	93.74% <ø> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
tasks.c	95.04% <ø> (+0.21%)	⬆️

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

[swaldhoer]

I am bin fan of using clang-format, so I happy to see this pull request.

However, I have two questions/concerns with this implementation:

• It seems like the action that is referenced just installs any latest available clang-format (see https://github.com/FreeRTOS/CI-CD-Github-Actions/blob/ef4c2fb5abe1869d06602b8a780c4dbee0d63635/formatting/action.yml#L38C61-L38C61). I would suggest to pin the exact clang-format version to be installed in the action, so that everything is reproducible at any later time.
• If the configuration file no longer is part of the https://github.com/FreeRTOS/CI-CD-Github-Actions/ repository, how can consistency between the projects be achieved? The versions of the .clang-format files always need to the match the clang-format version (whether it is specified/pinned or not). I think it would be easier to have one configuration in the CI/CD repository like it was done with uncrustify.

We are consuming multiple FreeRTOS projects into our project and we enforce that these files are not reformatted, instead keep the FreeRTOS style so that updating and seeing the actual changes is simpler.
So for us it would be the most simple if the style would be kept in sync with all other FreeRTOS project so that we could change our workflow from uncrusifty to something like:

xyz-project/src/our-sources
...
xyz-project/src/FreeRTOS/.clang-format # matching version of the file copied from the CI/CD repository
xyz-project/src/FreeRTOS/FreeRTOS-Kernel
xyz-project/src/FreeRTOS/FreeRTOS-Plus-TCP

and we could simply run clang-format on the xyz-project/src/FreeRTOS/ directory.

What is your opinion on this?

[RichardBarry]

Formatting here looks incorrect.

[Skptak]

I am big fan of using clang-format, so I happy to see this pull request.

Hey @swaldhoer, glad to hear that you'd also be interested in using clang-format in this repo!

However, I have two questions/concerns with this implementation:

* It seems like the action that is referenced just installs any latest available clang-format (see https://github.com/FreeRTOS/CI-CD-Github-Actions/blob/ef4c2fb5abe1869d06602b8a780c4dbee0d63635/formatting/action.yml#L38C61-L38C61). I would suggest to pin the exact clang-format version to be installed in the action, so that everything is reproducible at any later time.

This is actually an intentional decision.

1. Clang-format supports backwards compatibility between a fair amount of major revisions. I actually formatted these files using clang-format version 16.
2. The idea is to try and be the most user friendly to people who are going to be working on this. By using the version of clang-format that ubuntu 22.04 installs when running sudo apt install clang-format I am hoping to run on the same version that a normal developer would be using. This is something we're not entirely sure if we want to do, or if we want to tie ourselves to a specific version of the tool.

* If the configuration file no longer is part of the https://github.com/FreeRTOS/CI-CD-Github-Actions/ repository, how can consistency between the projects be achieved? The versions of the `.clang-format` files always need to the match the clang-format version (whether it is specified/pinned or not). I think it would be easier to have one configuration in the CI/CD repository like it was done with uncrustify.

This is also an intentional choice.
We want to provide the .clang-format file in each repo so that users can just run
clang-format -i <MY_FILES> (or the exact formatting command that our CI-CD runs, which is printed out in the workflow log) before making a PR, and know that it will then pass the formatting check.

We will then maintain that all repos that use the CI-CD-Github-Actions/clang-formatting action use the same format file by running a check against all onboarded repos, as seen here. This way if the "common" .clang-format file is modified, in such a way that it changes the formatting of the files in a repo, we instantly know that the repos need to have their individual ones updated.

We are consuming multiple FreeRTOS projects into our project and we enforce that these files are not reformatted, instead keep the FreeRTOS style so that updating and seeing the actual changes is simpler. So for us it would be the most simple if the style would be kept in sync with all other FreeRTOS project so that we could change our workflow from uncrusifty to something like:

xyz-project/src/our-sources
...
xyz-project/src/FreeRTOS/.clang-format # matching version of the file copied from the CI/CD repository
xyz-project/src/FreeRTOS/FreeRTOS-Kernel
xyz-project/src/FreeRTOS/FreeRTOS-Plus-TCP

and we could simply run clang-format on the xyz-project/src/FreeRTOS/ directory.

What is your opinion on this?

First off I'm glad to hear that you're consuming multiple FreeRTOS projects and repository in your project!

And I've got some good news for you then!
While we are still using uncrustify for our code at this point in time, all FreeRTOS repos that are included in FreeRTOS-LTS now are formatted using an identical uncrustify config file, as seen on this workflow run

Additionally, using the FreeRTOS/CI-CD-Github-Action formatting check now provides a git patch that can be manually downloaded and applied to your code base.

OR! Once #787 gets merged in, you could actually copy the formatting bot github workflow file from this repository and use that to apply your formatting automatically. This bot is already applied to the other FreeRTOS-LTS repos mentioned before.

Where think the above should help you with maintaining code consistency with your projects?

However, to answer your question about clang-format itself:

The reason we haven't merged these changes across our repository is due to worries about the way clang-format handles some specific cases:

1. FreeRTOS-Kernel files currently use a lot of end of line comments, such as this example. Clang-format's wrapping of these means these comments have to be manually moved to be above the variable, which some people find to be less readable. Where this really starts to matter is when looking at comments that are used for things like static analysis comments:
   Current Uncrustify version: tasks.c:xTaskCreateStatic:1150

    TaskHandle_t xTaskCreateStatic( TaskFunction_t pxTaskCode,
                                    const char * const pcName, /*lint !e971 Unqualified char types are allowed for strings and single characters only. */
                                    const uint32_t ulStackDepth,

Clang-format version: tasks.c:xTaskCreateStatic:1278

TaskHandle_t xTaskCreateStatic( TaskFunction_t pxTaskCode,
                                const char * const pcName, /*lint !e971 Unqualified char
                                                              types are allowed for
                                                              strings and single
                                                              characters only. */
                                const uint32_t ulStackDepth,

(another really rough looking example of this can be found here, if you're curious)

2. There isn't a way, that I am aware of, to force clang-format to put all parameters into a function or a struct on a newline. There are a couple of issues raised into the llvm-project about this (Example one, and example two). This is relevant because we're worried about how some of our longer function and structure names, that don't cause the wrapping, to then look. This can be seen a bit better when doing something like clang-formatting a TCB struct for an MPU enabled task:

    TaskParameters_t xROAccessTaskParameters =
    {
        .pvTaskCode     = prvROAccessTask,
        .pcName         = "ROAccess",
        .usStackDepth   = configMINIMAL_STACK_SIZE,
        .pvParameters   = NULL,
        .uxPriority     = tskIDLE_PRIORITY,
        .puxStackBuffer = xROAccessTaskStack,
        .xRegions       =
        {
            { ( void * ) ucSharedMemory,       32, tskMPU_REGION_READ_ONLY | tskMPU_REGION_EXECUTE_NEVER  },
            #if ( configTOTAL_MPU_REGIONS == 16 )
            { ( void * ) ucSharedMemory1,      32, tskMPU_REGION_READ_ONLY | tskMPU_REGION_EXECUTE_NEVER  },
            { ( void * ) ucSharedMemory2,      32, tskMPU_REGION_READ_ONLY | tskMPU_REGION_EXECUTE_NEVER  },
            { ( void * ) ucSharedMemory3,      32, tskMPU_REGION_READ_ONLY | tskMPU_REGION_EXECUTE_NEVER  },
            { ( void * ) ucSharedMemory4,      32, tskMPU_REGION_READ_ONLY | tskMPU_REGION_EXECUTE_NEVER  },
            { ( void * ) ucSharedMemory5,      32, tskMPU_REGION_READ_ONLY | tskMPU_REGION_EXECUTE_NEVER  },
            { ( void * ) ucSharedMemory6,      32, tskMPU_REGION_READ_ONLY | tskMPU_REGION_EXECUTE_NEVER  },
            { ( void * ) ucSharedMemory7,      32, tskMPU_REGION_READ_ONLY | tskMPU_REGION_EXECUTE_NEVER  },
            { ( void * ) ucSharedMemory8,      32, tskMPU_REGION_READ_ONLY | tskMPU_REGION_EXECUTE_NEVER  },
            #endif /* configTOTAL_MPU_REGIONS == 16 */
            { ( void * ) ucROTaskFaultTracker, 32, tskMPU_REGION_READ_WRITE | tskMPU_REGION_EXECUTE_NEVER },
            { 0,                               0,  0                                                      },
        }
    };

Which then changes to:

    TaskParameters_t xROAccessTaskParameters = { .pvTaskCode = prvROAccessTask,
                                                 .pcName = "ROAccess",
                                                 .usStackDepth = configMINIMAL_STACK_SIZE,
                                                 .pvParameters = NULL,
                                                 .uxPriority = tskIDLE_PRIORITY,
                                                 .puxStackBuffer = xROAccessTaskStack,
                                                 .xRegions = {
                                                     { ( void * ) ucSharedMemory,
                                                       32,
                                                       tskMPU_REGION_READ_ONLY |
                                                           tskMPU_REGION_EXECUTE_NEVER },
    #if( configTOTAL_MPU_REGIONS == 16 )
                                                     { ( void * ) ucSharedMemory1,
                                                       32,
                                                       tskMPU_REGION_READ_ONLY |
                                                           tskMPU_REGION_EXECUTE_NEVER },
                                                     { ( void * ) ucSharedMemory2,
                                                       32,
                                                       tskMPU_REGION_READ_ONLY |
                                                           tskMPU_REGION_EXECUTE_NEVER },
                                                     { ( void * ) ucSharedMemory3,
                                                       32,
                                                       tskMPU_REGION_READ_ONLY |
                                                           tskMPU_REGION_EXECUTE_NEVER },
                                                     { ( void * ) ucSharedMemory4,
                                                       32,
                                                       tskMPU_REGION_READ_ONLY |
                                                           tskMPU_REGION_EXECUTE_NEVER },
                                                     { ( void * ) ucSharedMemory5,
                                                       32,
                                                       tskMPU_REGION_READ_ONLY |
                                                           tskMPU_REGION_EXECUTE_NEVER },
                                                     { ( void * ) ucSharedMemory6,
                                                       32,
                                                       tskMPU_REGION_READ_ONLY |
                                                           tskMPU_REGION_EXECUTE_NEVER },
                                                     { ( void * ) ucSharedMemory7,
                                                       32,
                                                       tskMPU_REGION_READ_ONLY |
                                                           tskMPU_REGION_EXECUTE_NEVER },
                                                     { ( void * ) ucSharedMemory8,
                                                       32,
                                                       tskMPU_REGION_READ_ONLY |
                                                           tskMPU_REGION_EXECUTE_NEVER },
    #endif /* configTOTAL_MPU_REGIONS == 16 */
                                                     { ( void * ) ucROTaskFaultTracker,
                                                       32,
                                                       tskMPU_REGION_READ_WRITE |
                                                           tskMPU_REGION_EXECUTE_NEVER },
                                                     { 0, 0, 0 },
                                                 } };

The team is working on finding out if there's a way for us to swap to clang-format while minimizing these risks, or if it's possible to tweak the clang-format file enough to solve these issues

If you have any suggestions for how to tweak our clang-format file to potentially solve some of these issues, or any other suggestions, it'd be greatly appreciated if you'd share those!

[swaldhoer]

Regarding 81fd37c: it is not necessary to pollute the code base with clang-format commands, you can just set ReflowComments to false, then comment only sections will not be changed. I tried this setting on the code snippet and it worked.

Regarding fixed clang-format version: The argument can be perfectly twisted around: You pin the version in the CI to the at this date latest version. Because auf backwards compatibility this should not lead to problems for contributes, but we have a defined version. Therefore, we get the both benefits: The version is defined, and the developer workflow is smooth.

For things like https://github.com/FreeRTOS/FreeRTOS-Kernel/pull/762/files?diff=unified&w=1#diff-325c4a46abe8fc694989d41460987b02695b3da183826ec9c317a444f1287a09L48 this I have not yet found a working solution for your configuration style. In our project, we move the comments above and set ReflowComments to false, as soon as a line break would be added. This results in:

static List_t xDelayedCoRoutineList1; /**< Delayed co-routines. */
/**< Delayed co-routines (two lists are used - one for delays that have overflowed the current tick count. */
static List_t xDelayedCoRoutineList2;
/**< Points to the delayed co-routine list currently being used. */
static List_t *pxDelayedCoRoutineList = NULL;
/**< Points to the delayed co-routine list currently being used to hold co-routines that have overflowed the current tick count. */
static List_t *pxOverflowDelayedCoRoutineList = NULL;
/**< Holds co-routines that have been readied by an external event.  They cannot be added directly to the ready lists as the ready lists cannot be accessed by interrupts. */
static List_t xPendingReadyCoRoutineList;

but as you said you don't want to have this style.

For the TaskHandle_t xTaskCreateStatic I was also not yet able to find a good configuration. It may be somehow possible by combing ReflowComments, PenaltyBreakComment, and PenaltyBreakOpenParenthesis. But gets tricky and might not catch every corner case, so maybe sometimes clang-format comments might still be needed.

Regarding the common clang-format style across repos: perfect, thanks.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[swaldhoer]

@Skptak Has clang-format as formatter for this project been dropped? What is the current formatting rule for the repository? If it has been dropped, is this something you will reconsider in the future or if not, what's the plan?

[Skptak]

Hey @swaldhoer, sorry for the delay
The team is still considering this work, but it has been delayed due to the code changes going into the Kernel for 10.6.1 and 10.6.2.
If this is a feature you feel strongly about would you please create a post on the FreeRTOS Forum asking about it? Having a post on the forum will allow the community at large to discuss it better :)