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 C++ codebase #1984

Open
2 tasks
thomas-bc opened this issue Apr 25, 2023 · 4 comments
Open
2 tasks

Format C++ codebase #1984

thomas-bc opened this issue Apr 25, 2023 · 4 comments
Assignees
Labels
EPIC Container for related issues High Priority High Priority issue that needs to be resolved.

Comments

@thomas-bc
Copy link
Collaborator

thomas-bc commented Apr 25, 2023

F´ Version devel
Affected Component N/A

Feature Description

We are going to start formatting the entire C++ codebase and enforce the formatting through a CI check.

Plan

  • Create CI workflow to check that C++ is formatted correctly, with an optional exclude list. Start by excluding everything.
  • Iteratively format the C++ codebase, removing the exclusion parameter as we go.

Subtasks

  • Create clang-format check workflow
  • Format each top-level folder within F´, to be broken down in other issues
@thomas-bc thomas-bc self-assigned this Apr 25, 2023
@LeStarch LeStarch added the EPIC Container for related issues label Nov 29, 2023
@LeStarch LeStarch added the High Priority High Priority issue that needs to be resolved. label Mar 27, 2024
@JohanBertrand
Copy link
Contributor

Is anyone working on that right now? I'm happy having a look at it

@thomas-bc
Copy link
Collaborator Author

thomas-bc commented May 28, 2024

@JohanBertrand if you don't mind I'll take the CI part of this ticket, because we have an idea of how we want to do it. I'm working on this right now, I'll tag you in the PR if you'd like to review it

Once the check is there, feel free to help out formatting the code base!

We will format by small chunks and activate the CI per-module as we go.

@thomas-bc
Copy link
Collaborator Author

One issue that is arising, and is loosely related to nasa/fprime-tools#199

clang-format, for some obscure reason, has some slight variations in the formatting it outputs across different versions of the tool (even when using the exact same style). I have seen it happen with indentation of comments, and spacing like this one: thomas-bc@a4585cd

Some conversations about it here and here

We are likely going to need to pick a version of the tool we want to use in CI, version that users will need to use if they need to pass CI. It is very inconvenient... but there doesn't seem to be a way around it.

Let me know if you can think of anything else

cc @LeStarch @JohanBertrand

@JohanBertrand
Copy link
Contributor

I think it's reasonable to fix the version of the formatter, like it is also done with the static analyser (clang-tidy-12). Worst case scenario, the minor differences between two formatted versions would be flagged by the CI, and it could be resolved in one single commit.

It might be good to select a version of clang-tidy which is the same as clang-format to make it easier for the developers.

I'm not sure which version to select however. clang-tidy-18 seem to have some new warnings and is detecting two new issues in the current configuration on the development branch. clang-format-14 is adding the absolute path for the style that we discussed in nasa/fprime-tools#199, which might be great to keep. The space difference that you mentioned as an example was introduced between clang-format-17 and clang-format-18.

Clang tidy/format 12 to 18 are easily available on ubuntu20.04 and 22.04 (I'm not sure about Mac OS), so any of those version would work for me.

For fprime-util format, we will need to decide if we want it to fail when the version is different than the one we expect. I guess we could send a warning during the formatting but still process the files if the version is different. What is your opinion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EPIC Container for related issues High Priority High Priority issue that needs to be resolved.
Projects
None yet
Development

No branches or pull requests

3 participants