RIOT-OS
tracking: replacing header guards with #pragma once
Description
~~Clang-format requires the header guard #ifndef #define HEADER_GUARD_H to be outside of the doxygen documentation, that means outside the @{ ... @} block. If this is not done, then clang-format will indent all preprocessor directives after the #ifndef. The problem is that the majority of RIOT's header files don't adhere to this order. They either have the header guard inside the doxygen block or the header guard starts correctly outside the doxygen block but the #endif directive is still inside the doxygen documentation. To fix this inconsistency, I have written a python script that recursively scans all header files in a directory and rearranges the parts, so that the correct order is established.~~
Previously this issue tracked the effort to bring the header guards into the correct order to work with clang-format. Recently it was decided that from now on, RIOT would use the #pragma once macro instead. This means that some of the previous pull request are outdated and every part of the RIOT repo needs to be updated.
Fixed directories
- [x] drivers #21496, OLD: ~~#21325~~
- [x] sys: #21497 OLD: ~~#21340~~
- [x] cpu: #21501
- [x] core: #21405
- [x] boards: #21504
- [x] examples: #21517
- [ ] pkg
- [ ] tests
Testing procedure
Use my python script and change the path variable 'RIOT_PATH' in the script to whatever directory you want to recursively fix the header files. Warning: the script isn't fool-proof but fixes 95% of all header files that have this issue. There are individual issues with the formatting that require manual intervention.
The script can be found here
Application
This is based on the fix in https://github.com/RIOT-OS/RIOT/pull/20905 addressed by @mguetschow but with the added benefit of keeping the blocks balanced.
The order should also be fixed in https://github.com/aabadie/riot-generator
I wonder if your Python script could be adapted and added to the static tests, so that for future contributions, the include guards are checked as well before merging 🤔
I wonder if your Python script could be adapted and added to the static tests, so that for future contributions, the include guards are checked as well before merging 🤔
This seem like a very reasonable addition. I have yet to look into CI stuff as I have never worked with it.
There is some documentation in the dist/tools/ci folder https://github.com/RIOT-OS/RIOT/tree/master/dist/tools/ci and you can look at how the check-scripts are called by dist/tools/ci/static_tests.sh and how they operate.
You don't necessarily have to look into CI for that, the script is executed when you call make static-test and that's exactly what the CI system does as well.
We had that discussion at VMA 2024.11 and had general consensus on switching to #pragma once, while having some reservation about the necessary migration work to keep in-tree consistency. Since we are now anyways touching a lot of header files, there was consensus at today's maintainer weekly that we can just as well use that work to switch over to #pragma once.
For #pragma once, the dist/tools/headerguards/headerguards.py script has to be modified, otherwise the static tests fail:
Hello, I am new to the RIOT community and would like to help fix the headers. I can start in the core folder if no one is working on that already. Also, I made a modified version of @KSKNico's script to replace the header guard entirely with #pragma once which may be helpful.
@mguetschow @crasbe Should vendor specific header files also be modified to use #pragma once?
If those are vendor-provided (aka just copied to the RIOT tree from somewhere else) then I wouldn't change them. But all other header files belonging to RIOT which happen to be for a specific vendor should be modified of course.
@KSKNico could you extend your script to check that if there is an empty line before and after the #ifndef...#define...#endif that one of these lines is also removed? That would avoid introducing empty line static errors.
See https://github.com/RIOT-OS/RIOT/pull/21501#issuecomment-2899145625 and #21502.
@crasbe I updated the script to account for that. Should I open another pull request?