Skip to content

Add clang-tidy checks and apply them #1989

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.

Sign up for GitHub

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

Jump to bottom
Merged
pcarruscag merged 9 commits into from
Apr 9, 2023
Merged

Add clang-tidy checks and apply them #1989

pcarruscag merged 9 commits into from
Apr 9, 2023

Conversation

@kursatyurt
Copy link
Contributor

@kursatyurt kursatyurt commented Mar 26, 2023

Proposed Changes

clang-tidy is a well-known tool. This PR adds clang-tidy file with common checks and applies to the code base.

PR Checklist

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson).
  • My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp), if necessary.

pcarruscag
pcarruscag reviewed Mar 26, 2023
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 26, 2023
View reviewed changes
Copy link
Contributor

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@kursatyurt
Copy link
Contributor Author

kursatyurt commented Mar 27, 2023 •
edited
Loading

@pcarruscag I would like to get your opinion about following checks since they are related to code style and changes many things

https://clang.llvm.org/extra/clang-tidy/checks/readability/braces-around-statements.html -> This can be disabled for short ones. Needs to decide short is how many characters.

https://clang.llvm.org/extra/clang-tidy/checks/readability/else-after-return.html

https://clang.llvm.org/extra/clang-tidy/checks/readability/isolate-declaration.html

May be this is too https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-auto.html

If chosen those ones can be applied just before release v8. to avoid merge conflicts

pcarruscag
pcarruscag reviewed Mar 31, 2023
View reviewed changes
@pcarruscag
Copy link
Member

pcarruscag commented Mar 31, 2023

Else after return is fine but I don't like the others.
Isolate declarations is not addressing the true problem. Variables should be declared when needed and made const if possible.
We cannot use auto for the return type of operations with su2double

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 31, 2023
View reviewed changes
Common/src/fem/fem_integration_rules.cpp
}

void CFEMStandardElementBase::IntegrationPointsTriangle(void) {
void CFEMStandardElementBase::IntegrationPointsTriangle() {

Check warning

Code scanning / CodeQL

Poorly documented large function

Poorly documented function: fewer than 2% comments for a function of 5388 lines.
Common/src/fem/fem_integration_rules.cpp
}

void CFEMStandardElementBase::IntegrationPointsTetrahedron(void) {
void CFEMStandardElementBase::IntegrationPointsTetrahedron() {

Check warning

Code scanning / CodeQL

Poorly documented large function

Poorly documented function: fewer than 2% comments for a function of 4733 lines.
Common/src/grid_movement/CSurfaceMovement.cpp

Xcoord.push_back(Airfoil_Coord[0]);
Ycoord.push_back(Airfoil_Coord[1] * factor * AirfoilScale);
Ycoord.emplace_back(Airfoil_Coord[1] * factor * AirfoilScale);

Check failure

Code scanning / CodeQL

Missing return-value check for a 'scanf'-like function

This variable is read, but may not have been written. It should be guarded by a check that the [call to scanf](1) returns at least 1.
pcarruscag
pcarruscag approved these changes Apr 9, 2023
View reviewed changes
@pcarruscag
Copy link
Member

pcarruscag commented Apr 9, 2023

I went over the changes very quickly, if someone else wants to take a look it would be nice.

@kursatyurt
Copy link
Contributor Author

kursatyurt commented Apr 9, 2023

@bigfooted, @tbellosta, @WallyMaier if any of you had time I would be great.

@bigfooted
Copy link
Contributor

bigfooted commented Apr 9, 2023

@bigfooted, @tbellosta, @WallyMaier if any of you had time I would be great.

I've had a look at the changes. I do not have strong opinions about the checks that were chosen (or not chosen), but I am glad that we have this now.

bigfooted
bigfooted approved these changes Apr 9, 2023
View reviewed changes
Copy link
Contributor

@bigfooted bigfooted left a comment

Choose a reason for hiding this comment

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

Great work!

@pcarruscag pcarruscag merged commit 52f2450 into develop Apr 9, 2023
@pcarruscag pcarruscag deleted the feature_clang-tidy branch April 9, 2023 21:29
@pr-triage pr-triage bot added the PR: merged label Apr 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bigfooted bigfooted bigfooted approved these changes

@pcarruscag pcarruscag pcarruscag approved these changes

Assignees

No one assigned

Labels

changelog:chore enhancement PR: merged PR: reviewed-approved PR: unreviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kursatyurt @pcarruscag @bigfooted