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

Update of clang tidy checks #2710

Merged
merged 3 commits into from
May 2, 2024

Conversation

JohanBertrand
Copy link
Contributor

@JohanBertrand JohanBertrand commented Apr 30, 2024

Related Issue(s)
Has Unit Tests (y/n)
Documentation Included (y/n)

Change Description

  • Add clang-tidy check on braces after statements and related flagged errors.
  • Merge release.clang-tidy into .clang-tidy and update CI. Unless it is used for somewhere externally, I do not see the file .clang-tidy used anywhere otherwise.
  • Also fix an issue in StringBase flagged on my docker image, but not on the CI?

Rationale

Enforce consistent coding standard

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

I am happy with these changes but I will want @Joshua-Anderson and @thomas-bc to weigh in.

Comment on lines +93 to +95
for ( ; index + 4 <= length; index += 4) {
addWordAligned(&data[index]);
}

Check warning

Code scanning / CodeQL

Unbounded loop Warning

This loop does not have a fixed bound.
@Joshua-Anderson
Copy link
Contributor

The logic for separating out .clang-tidy and release.clang-tidy was because we had some clang tidy checks we wanted to enforce for flight code, but not project unit tests.

See this github workflow where release.clang-tidy is used

At this moment, we could remove release.clang-tidy if we were okay with disallowing recursion on unit tests, but there may be future checks we only want to enforce on flight code.

@JohanBertrand
Copy link
Contributor Author

@Joshua-Anderson Thank you for the explanation, I am happy keeping two different files. I did not realize that the first call of clang-tidy on the CI was also running on the unit tests.

@LeStarch
Copy link
Collaborator

LeStarch commented May 1, 2024

@JohanBertrand I think we will need to go with @Joshua-Anderson's suggestion. The failure we see is recursion used in a UT.

@JohanBertrand
Copy link
Contributor Author

I reverted the clang-tidy configuration file changes and added some comments. It should be good to go!

Copy link
Collaborator

@LeStarch LeStarch left a comment

Choose a reason for hiding this comment

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

Looks good.

@LeStarch LeStarch merged commit eb518e9 into nasa:devel May 2, 2024
34 checks passed
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.

3 participants