This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check
Needs ReviewPublic

Authored by Izaron on Feb 1 2022, 4:01 PM.

Details

Summary

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.

Diff Detail

Event Timeline

Izaron created this revision.Feb 1 2022, 4:01 PM
Izaron requested review of this revision.Feb 1 2022, 4:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2022, 4:01 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Izaron added a comment.EditedFeb 1 2022, 4:11 PM

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

Eugene.Zelenko added inline comments.
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
6 ↗(On Diff #405116)

Please synchronize first statement with Release Notes.

53 ↗(On Diff #405116)

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
order of "most likely encountered first" to get an early out
when matching?

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
6 ↗(On Diff #405116)

...const variable definitions...const variable declarations

10 ↗(On Diff #405116)

...variables are a deprecated way...

Izaron updated this revision to Diff 416347.Mar 17 2022, 3:32 PM
Izaron marked 7 inline comments as done.

Fix issues

Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 3:32 PM
Izaron added inline comments.Mar 17 2022, 3:33 PM
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp
65–69

Thanks, it is more convenient!

reordering these unless() clauses in order of "most likely encountered first"

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.

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

Thanks, this is excellent documentation! I learned a plenty of new things.

LegalizeAdulthood requested changes to this revision.Jun 22 2022, 3:06 PM

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
This revision now requires changes to proceed.Jun 22 2022, 3:06 PM
njames93 added inline comments.Jun 22 2022, 11:21 PM
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp
65–69

does unless(isVarInline(), isExternC()) mean unless(isVarInline() && isExternC()) or unless(isVarInline() || isExternC())
This kind of ambiguity could be addressed with unlessAllOf and unlessAnyOf but I don't think its worth the effort.

Izaron updated this revision to Diff 466660.Oct 10 2022, 4:17 PM

Rebased onto main (after a long long time).

Thanks to @LegalizeAdulthood for instructions!

Eugene.Zelenko added inline comments.Oct 10 2022, 4:23 PM
clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst
6

Excessive newline.

Izaron updated this revision to Diff 466661.Oct 10 2022, 4:28 PM

Removed excessive newline.

Thanks to @Eugene.Zelenko!

Izaron marked an inline comment as done.Oct 10 2022, 4:28 PM

A friendly ping 🙂

Lounarok added a comment.EditedDec 1 2022, 5:26 PM

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).

Izaron added a comment.Jan 1 2023, 6:50 PM

I tried this patch and it's really helpful!

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)

carlosgalvezp added inline comments.Jan 2 2023, 2:31 AM
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...

Izaron updated this revision to Diff 498637.Feb 18 2023, 3:25 PM

Use the common isInAnonymousNamespace matcher.

Split the test into two files.

Thanks to @carlosgalvezp for advices!

Izaron marked 2 inline comments as done.Feb 18 2023, 3:26 PM
Izaron added inline comments.
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.

LegalizeAdulthood resigned from this revision.Mar 29 2023, 9:06 AM