C++17 inline variables are far more convenient compared with
extern const variables declarations, and in general are more ODR-safe and
memory efficient compared with non-inline const variables definitions.
It is a part of a number of code styles to use C++17 inline variables.
The checker would help users to follow this code style.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
For example, Google Code Style strongly advises C++17 inline variables. There are company guides how to use them: https://abseil.io/tips/168, https://abseil.io/tips/140. I believe other codestyles also use the feature.
P.S. If this review is eventually approved, kindly please merge the commit on my behalf =) As I don't have merge access. My name is Evgeny Shulgin and email is izaronplatz@gmail.com. Sorry for inconvenience!
UPD: I've got merge access, so the paragraph above is irrelevant
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h | ||
---|---|---|
34 | I think this is excessive comment. It's regular convention. | |
clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst | ||
7 | Please synchronize first statement with Release Notes. | |
54 | Please highlight option values in this section with single back-ticks. |
You've probably had this check in development for some time, but
we just updated the documentation for contributing to clang-tidy
and it might help to go over it for your new check to see if there
is anything that stands out.
https://clang.llvm.org/extra/clang-tidy/Contributing.html
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp | ||
---|---|---|
65–69 | Comment: Why don't we have a variadic unless(...) matcher? Is there any utility to reordering these unless() clauses in | |
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h | ||
20 | This block comment mentions functions, but the documentation only mentions variables. | |
clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst | ||
7 | ...const variable definitions...const variable declarations | |
11 | ...variables are a deprecated way... |
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp | ||
---|---|---|
65–69 | Thanks, it is more convenient!
I reordered them based on my own approximal estimation. We could make a benchmark to observe the impact of different orders, but it goes beyond this patch. | |
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h | ||
20 | Oops, that's a typo: of course there are nothing abouth functions anywhere in the checker. |
Clang-Tidy tests and docs have moved to module subdirectories.
Please rebase this onto main:HEAD and:
- fold your changes into the appropriate subdirs, stripping the module prefix from new files.
- make the target check-clang-extra to validate your tests
- make the target docs-clang-tools-html to validate any docs changes
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp | ||
---|---|---|
65–69 | does unless(isVarInline(), isExternC()) mean unless(isVarInline() && isExternC()) or unless(isVarInline() || isExternC()) |
Rebased onto main (after a long long time).
Thanks to @LegalizeAdulthood for instructions!
clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst | ||
---|---|---|
5 ↗ | (On Diff #466660) | Excessive newline. |
I tried this patch and it's really helpful!
However I found that it warns when it comes to constexpr static variable.
Snippet: constexpr static int UNDEFINED_ERROR{0};
Warning msg: warning: global constant 'UNDEFINED_ERROR' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
According to this, "A constexpr specifier used in a function or static data member (since C++17) declaration implies inline."
It seems it should not warn on constexpr static variable.
Just a notification up here, I'm fine with // NOLINTNEXTLINE(modernize-use-inline-const-variables-in-headers).
You're welcome!
According to this, "A constexpr specifier used in a function or static data member (since C++17) declaration implies inline."
That's correct, for functions and static data members.
However I found that it warns when it comes to constexpr static variable.
Snippet: constexpr static int UNDEFINED_ERROR{0};
Warning msg: warning: global constant 'UNDEFINED_ERROR' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
According to this, "A constexpr specifier used in a function or static data member (since C++17) declaration implies inline."
It seems it should not warn on constexpr static variable.
UNDEFINED_ERROR is neither a function nor a static data member. Therefore a warning is meaningful. There is no inline implication in your case.
(data member is a variable inside a struct/class, but not a "freestanding" one)
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp | ||
---|---|---|
22–24 | I recently added this matcher into ASTMatchers, so you can remove this custom matcher :) | |
clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp | ||
1 ↗ | (On Diff #466661) | Should tests be added for the CheckNonInline and CheckExtern options? |
(data member is a variable inside a struct/class, but not a "freestanding" one)
Thanks for pointing that out. I misunderstood what data member is...
Use the common isInAnonymousNamespace matcher.
Split the test into two files.
Thanks to @carlosgalvezp for advices!
clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp | ||
---|---|---|
1 ↗ | (On Diff #466661) | I decided to split the test into two files as it is done for other checkers that have multiple options. |
This block comment mentions functions, but the documentation only mentions variables.