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

Clang-tidy CI test: bump version from 15 to 16 #5592

Merged

Conversation

lucafedeli88
Copy link
Member

@lucafedeli88 lucafedeli88 commented Jan 22, 2025

This PR bumps the version used for clang-tidy CI tests from 15 to 16.
It also addresses all the issues found with the upgraded tool.

Most of the issues are related to this newly introduced check:

  • cppcoreguidelines-avoid-const-or-ref-data-members
    The check enforces CppCoreGuidelines about constant and reference data members .
    In general, I understand the argument against using constant or reference data members.
    There is however one case in which I am not fully convinced by the suggestion of the tool: in PML.H const amrex::BoxArray& m_grids; becomes amrex::BoxArray m_grids; and I am wondering if this can be an issue for performances. Maybe we could consider using a (possibly smart) pointer to the BoxArray, instead of making a copy. (we are now using a pointer for amrex::BoxArray m_grids;).

Few issues were instead related to these checks:

  • modernize-loop-convert
    This check was already enabled, but clang-tidy-16 broadens its scope with respect to previous versions.

  • modernize-use-auto
    Only one case. I am a bit confused because this should have been found also by version 15 of the tool.

  • misc-use-anonymous-namespace
    This is a new check. Actually, the issues found with this check were false positives, but they disappeared when I properly set misc-use-anonymous-namespace.HeaderFileExtensions in the .clang-tidy configuration file to recognize .H files as headers.

  • misc-misplaced-const
    Only one case. I am a bit confused because this should have been found also by version 15 of the tool.

  • readability-misleading-indentation [NOW DISABLED DUE TO FALSE POSITIVES]
    This check was already enabled. However, with this newer version of the tool, it seems to me that it generates some false positives. Therefore, I would like to propose to disable it. We may try to re-enable it when we will bump the version from 16 to 17.

@lucafedeli88 lucafedeli88 added component: tests Tests and CI cleaning Clean code, improve readability labels Jan 22, 2025
@lucafedeli88 lucafedeli88 changed the title [WIP] Clang-tidy CI test: bump version from 15 to 16 Clang-tidy CI test: bump version from 15 to 16 Jan 22, 2025
@lucafedeli88 lucafedeli88 requested review from ax3l, RemiLehe and EZoni and removed request for ax3l and RemiLehe January 23, 2025 09:10
@EZoni
Copy link
Member

EZoni commented Jan 23, 2025

@lucafedeli88

Is there a specific reason why you want to move to version 16 as opposed to even newer versions?

@lucafedeli88
Copy link
Member Author

lucafedeli88 commented Jan 23, 2025

@lucafedeli88

Is there a specific reason why you want to move to version 16 as opposed to even newer versions?

@EZoni , the main reason is to split into two PRs the changes that will be required with newer versions of clang-tidy . This is also motivated by the fact that clang-tidy-17 introduces a fair amount of new checks ! I have already opened a (WIP) PR to upgrade from 16 to 17 ( #5600 ). I expect that it will be finished by next week.
However, if you prefer a single PR, I can bump the version to 17 and address the issues that clang-tidy-17 will find.

@@ -112,7 +112,7 @@ public:
}

private:
const amrex::BoxArray& m_grids;
amrex::BoxArray m_grids;
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply a ref?

Suggested change
amrex::BoxArray m_grids;
amrex::BoxArray & m_grids;

Copy link
Member Author

Choose a reason for hiding this comment

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

@ax3l , the goal of avoid-const-or-ref-data-members is to avoid using const or ref data members, on the grounds that

Having such members is rarely useful, and makes the class only copy-constructible but not copy-assignable

This recommendation is part of the C++ Core Guidelines, and clant-tidy documentation suggests also this additional reading on the topic.

That said, we are not forced to enforce this rule in WarpX (although I think that this rule makes sense in most cases), or we could make an exception specifically for this line. For the moment I've implemented @WeiqunZhang 's suggestion, but I can revert it and just add a NOLINT.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, but my suggestion just adds & not const back :D

Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ax3l ax3l merged commit 547bfbb into ECP-WarpX:development Jan 29, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleaning Clean code, improve readability component: tests Tests and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants