Skip to content
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

Format code using clang format and introduce CI checks for it #103

Merged
merged 10 commits into from
Nov 13, 2023

Conversation

josecm
Copy link
Member

@josecm josecm commented Oct 20, 2023

This PR is initially marked as a draft to achieve consensus on the formatting rules defined in the bao-project/bao-ci .clang_format file. I'd ask the @bao-project/core-team to scan the code base and to provide feedback in this sense if they don't agree with any specific previously discussed format decisions so we can discuss further and update the clang-format options accordingly.

Temporarily the ci branch is set to bao-project/bao-ci's exp/clang-format-options branch so that we can play around with the CI options before merging them into bao-project/bao-ci 's main accordingly and set the submodule in this PR to it.

Depends on bao-project/bao-ci#70.

Instantiate the code formating make rule. Also delete the original
.clang-format file because this configuration is out-of-date and the ci
repo provides an updated one.

Signed-off-by: Jose Martins <[email protected]>
This commits also adds a status badge in the README.md file for the
overall code quality workflow which is intended to have additional
static code analysis checks in the future.

Signed-off-by: Jose Martins <[email protected]>
@josecm josecm force-pushed the style/clang_format branch from 1318d66 to 4efe0b9 Compare October 20, 2023 18:42
@danielRep danielRep self-assigned this Oct 21, 2023
@josecm josecm force-pushed the style/clang_format branch 2 times, most recently from c3ebbb0 to 426baad Compare October 21, 2023 14:26
Copy link
Member

@danielRep danielRep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the overall, I agree with clang format options that we are using. I tried to keep my eyes wide open to check for inconsistencies on the formatted files, but for sure we need to run this in all boards.

Regarding the clang options:

  • SpaceBeforeParensOptions: My a personal taste, I wouldn't use the space before control statements if we are not using it for functions
  • AlignTrailingComments: this options also controls the comments in structs right? If so, I would force it. In large structs keeping the comments unaligned does not help the readability of each member of the struct.

src/arch/armv8/armv8-a/inc/arch/page_table.h Show resolved Hide resolved
src/platform/drivers/pl011_uart/inc/drivers/pl011_uart.h Outdated Show resolved Hide resolved
src/platform/drivers/pl011_uart/inc/drivers/pl011_uart.h Outdated Show resolved Hide resolved
@josecm josecm force-pushed the style/clang_format branch from 426baad to 5c882bd Compare October 21, 2023 16:50
@josecm
Copy link
Member Author

josecm commented Oct 21, 2023

I'm thinking we could define a max of 100 columns instead of 80. This is kind of being stuck in the past... Modern monitors are wide enough that this does not become an hassle.

@danielRep
Copy link
Member

I'm thinking we could define a max of 100 columns instead of 80. This is kind of being stuck in the past... Modern monitors are wide enough that this does not become an hassle.

I totally agree with this. Other projects already follow the 100 columns too.

DavidMCerdeira
DavidMCerdeira previously approved these changes Oct 27, 2023
src/arch/armv8/aborts.c Outdated Show resolved Hide resolved
danielRep
danielRep previously approved these changes Oct 27, 2023
@josecm josecm dismissed stale reviews from danielRep and DavidMCerdeira via a2c0176 October 27, 2023 18:35
@josecm josecm force-pushed the style/clang_format branch from 5c882bd to a2c0176 Compare October 27, 2023 18:35
@josecm
Copy link
Member Author

josecm commented Oct 27, 2023

Updated to use a 100 char column limit.

@josecm
Copy link
Member Author

josecm commented Nov 5, 2023

If no one has further comments, I bumping this to full PR.

@josecm josecm marked this pull request as ready for review November 5, 2023 15:54
@josecm
Copy link
Member Author

josecm commented Nov 5, 2023

We need to first merge bao-project/bao-ci#70, then update the ci submodule here, and only then can we merge this.

src/core/mem.c Outdated Show resolved Hide resolved
src/core/vm.c Outdated Show resolved Hide resolved
danielRep
danielRep previously approved these changes Nov 6, 2023
DavidMCerdeira
DavidMCerdeira previously approved these changes Nov 6, 2023
@josecm
Copy link
Member Author

josecm commented Nov 6, 2023

Updated CI submodule.

danielRep
danielRep previously approved these changes Nov 6, 2023
@danielRep danielRep force-pushed the style/clang_format branch 2 times, most recently from 59fecb5 to 9934d08 Compare November 6, 2023 15:47
Signed-off-by: Daniel Oliveira <[email protected]>
Signed-off-by: Jose Martins <[email protected]>
@josecm josecm force-pushed the style/clang_format branch from 9934d08 to 88dd4f0 Compare November 7, 2023 13:32
@DavidMCerdeira DavidMCerdeira self-requested a review November 9, 2023 11:37
@danielRep danielRep merged commit 51e8fac into main Nov 13, 2023
13 checks passed
@danielRep danielRep deleted the style/clang_format branch November 13, 2023 15:11
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.

5 participants