[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron marked 2 inline comments as done.
Izaron added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp:1
+// RUN: %check_clang_tidy %s -std=c++17 
modernize-use-inline-const-variables-in-headers %t
+

carlosgalvezp wrote:
> Should tests be added for the `CheckNonInline` and `CheckExtern` options?
I decided to split the test into two files as it is done for other checkers 
that have multiple options.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 498637.
Izaron added a comment.

Use the common `isInAnonymousNamespace` matcher.

Split the test into two files.

Thanks to @carlosgalvezp for advices!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers-extern.hpp
  
clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers-non-inline.hpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers-non-inline.hpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers-non-inline.hpp
@@ -0,0 +1,42 @@
+// RUN: %check_clang_tidy %s -std=c++17 modernize-use-inline-const-variables-in-headers %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: modernize-use-inline-const-variables-in-headers.CheckNonInline, value: true}, \
+// RUN:  {key: modernize-use-inline-const-variables-in-headers.CheckExtern, value: false}]}"
+
+const int ConstFoo = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: global constant 'ConstFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
+// CHECK-FIXES: {{^}}inline const int ConstFoo = 1;{{$}}
+
+namespace N {
+constexpr int NamespaceFoo = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: global constant 'NamespaceFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
+// CHECK-FIXES: {{^}}inline constexpr int NamespaceFoo = 1;{{$}}
+} // namespace N
+
+struct S {
+  // no warning: the variable is not at file scope
+  static const int StructFoo = 1;
+};
+
+// no warning: non-const global variables have external linkage
+int NonConstFoo = 1;
+
+// no warning: volatile global variables have external linkage
+const volatile int VolatileFoo = 1;
+
+// no warning: templates and their instantiations have external linkage
+template 
+const auto TemplateFoo = sizeof(T);
+
+// no warning: already fixed
+inline const int InlineFoo = 1;
+
+// no warning: C has no 'inline variables'
+extern "C" {
+const int CFoo0 = 1;
+}
+
+// no warning: 'inline' is invisible when within an unnamed namespace
+namespace {
+const int AnonNamespaceFoo = 1;
+} // namespace
Index: clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers-extern.hpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers-extern.hpp
@@ -0,0 +1,10 @@
+// RUN: %check_clang_tidy %s -std=c++17 modernize-use-inline-const-variables-in-headers %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN: [{key: modernize-use-inline-const-variables-in-headers.CheckNonInline, value: false}, \
+// RUN:  {key: modernize-use-inline-const-variables-in-headers.CheckExtern, value: true}]}"
+
+extern const int ExternFoo;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: global constant 'ExternFoo' should be converted to C++17 'inline variable' [modernize-use-inline-const-variables-in-headers]
+
+// no warning: C has no 'inline variables'
+extern "C" const int CFoo1 = 1;
Index: clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst
@@ -0,0 +1,68 @@
+.. title:: clang-tidy - modernize-use-inline-const-variables-in-headers
+
+modernize-use-inline-const-variables-in-headers
+===
+
+Suggests switching to C++17 ``inline variables`` for non-inline const
+variable definitions and extern const variable declarations in header files.
+Non-inline const variables make a separate instance of the variable for each
+translation unit that includes the header, which can lead to subtle violation
+of the ODR. Extern const variables are a deprecated way to define a constant
+since C++17.
+
+.. code-block:: c++
+
+   // Foo.h
+   const int ConstFoo = 1; // Warning: should be marked as 'inline'
+
+   namespace N {
+ constexpr int NamespaceFoo = 1; // Warning: should be marked as 'inline'
+   }
+
+   extern const int ExternFoo; // 

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Lounarok via Phabricator via cfe-commits]

Lounarok added a comment.

> (//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...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp:21-23
+AST_MATCHER(NamedDecl, isInAnonymousNamespace) {
+  return Node.isInAnonymousNamespace();
+}

I recently added this matcher into ASTMatchers, so you can remove this custom 
matcher :) 



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp:1
+// RUN: %check_clang_tidy %s -std=c++17 
modernize-use-inline-const-variables-in-headers %t
+

Should tests be added for the `CheckNonInline` and `CheckExtern` options?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

In D118743#3965423 , @Lounarok wrote:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Lounarok via Phabricator via cfe-commits]

Lounarok added a comment.

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

Just a notification up here, I'm fine with `// constexpr implies inline, just 
ignore inline const warning
// NOLINTNEXTLINE(modernize-use-inline-const-variables-in-headers)`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

A friendly ping 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 41.
Izaron added a comment.

Removed excessive newline.

Thanks to @Eugene.Zelenko!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s -std=c++17 modernize-use-inline-const-variables-in-headers %t
+
+const int ConstFoo = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: global constant 'ConstFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
+// CHECK-FIXES: {{^}}inline const int ConstFoo = 1;{{$}}
+
+namespace N {
+constexpr int NamespaceFoo = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: global constant 'NamespaceFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
+// CHECK-FIXES: {{^}}inline constexpr int NamespaceFoo = 1;{{$}}
+} // namespace N
+
+extern const int ExternFoo;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: global constant 'ExternFoo' should be converted to C++17 'inline variable' [modernize-use-inline-const-variables-in-headers]
+
+struct S {
+  // no warning: the variable is not at file scope
+  static const int StructFoo = 1;
+};
+
+// no warning: non-const global variables have external linkage
+int NonConstFoo = 1;
+
+// no warning: volatile global variables have external linkage
+const volatile int VolatileFoo = 1;
+
+// no warning: templates and their instantiations have external linkage
+template 
+const auto TemplateFoo = sizeof(T);
+
+// no warning: already fixed
+inline const int InlineFoo = 1;
+
+// no warning: C has no 'inline variables'
+extern "C" {
+const int CFoo0 = 1;
+}
+extern "C" const int CFoo1 = 1;
+
+// no warning: 'inline' is invisible when within an unnamed namespace
+namespace {
+const int AnonNamespaceFoo = 1;
+} // namespace
Index: clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst
@@ -0,0 +1,68 @@
+.. title:: clang-tidy - modernize-use-inline-const-variables-in-headers
+
+modernize-use-inline-const-variables-in-headers
+===
+
+Suggests switching to C++17 ``inline variables`` for non-inline const
+variable definitions and extern const variable declarations in header files.
+Non-inline const variables make a separate instance of the variable for each
+translation unit that includes the header, which can lead to subtle violation
+of the ODR. Extern const variables are a deprecated way to define a constant
+since C++17.
+
+.. code-block:: c++
+
+   // Foo.h
+   const int ConstFoo = 1; // Warning: should be marked as 'inline'
+
+   namespace N {
+ constexpr int NamespaceFoo = 1; // Warning: should be marked as 'inline'
+   }
+
+   extern const int ExternFoo; // Warning: should be converted to C++17 'inline variable'
+
+   struct S {
+ static const int StructFoo = 1; // OK: the variable is not at file scope
+   };
+
+   int NonConstFoo = 1; // No warning: non-const global variables have external linkage
+
+   const volatile int VolatileFoo = 1; // No warning: volatile global variables have external linkage
+
+   template
+   const auto TemplateFoo = sizeof(T); // OK: templates and their instantiations have external linkage
+
+   inline const int InlineFoo = 1; // no warning: already fixed
+
+   // No warning: C has no 'inline variables'
+   extern "C" {
+ const int CFoo0 = 1;
+   }
+   extern "C" const int CFoo1 = 1;
+
+   // No warning: 'inline' is invisible when within an unnamed namespace
+   namespace {
+ const int AnonNamespaceFoo = 1;
+   }
+
+Options
+---
+
+.. option:: HeaderFileExtensions
+
+   A comma-separated list of filename extensions of header files (the filename
+   extensions should not include "." prefix). Default is `h,hh,hpp,hxx`.
+   For header files without an extension, use an empty string (if there are no
+   other desired extensions) or leave an empty element in the list. 

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst:5
+===
+
+

Excessive newline.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 40.
Izaron added a comment.

Rebased onto main (after a long long time).

Thanks to @LegalizeAdulthood for instructions!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/use-inline-const-variables-in-headers.hpp
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s -std=c++17 modernize-use-inline-const-variables-in-headers %t
+
+const int ConstFoo = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: global constant 'ConstFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
+// CHECK-FIXES: {{^}}inline const int ConstFoo = 1;{{$}}
+
+namespace N {
+constexpr int NamespaceFoo = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: global constant 'NamespaceFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
+// CHECK-FIXES: {{^}}inline constexpr int NamespaceFoo = 1;{{$}}
+} // namespace N
+
+extern const int ExternFoo;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: global constant 'ExternFoo' should be converted to C++17 'inline variable' [modernize-use-inline-const-variables-in-headers]
+
+struct S {
+  // no warning: the variable is not at file scope
+  static const int StructFoo = 1;
+};
+
+// no warning: non-const global variables have external linkage
+int NonConstFoo = 1;
+
+// no warning: volatile global variables have external linkage
+const volatile int VolatileFoo = 1;
+
+// no warning: templates and their instantiations have external linkage
+template 
+const auto TemplateFoo = sizeof(T);
+
+// no warning: already fixed
+inline const int InlineFoo = 1;
+
+// no warning: C has no 'inline variables'
+extern "C" {
+const int CFoo0 = 1;
+}
+extern "C" const int CFoo1 = 1;
+
+// no warning: 'inline' is invisible when within an unnamed namespace
+namespace {
+const int AnonNamespaceFoo = 1;
+} // namespace
Index: clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/modernize/use-inline-const-variables-in-headers.rst
@@ -0,0 +1,69 @@
+.. title:: clang-tidy - modernize-use-inline-const-variables-in-headers
+
+modernize-use-inline-const-variables-in-headers
+===
+
+
+Suggests switching to C++17 ``inline variables`` for non-inline const
+variable definitions and extern const variable declarations in header files.
+Non-inline const variables make a separate instance of the variable for each
+translation unit that includes the header, which can lead to subtle violation
+of the ODR. Extern const variables are a deprecated way to define a constant
+since C++17.
+
+.. code-block:: c++
+
+   // Foo.h
+   const int ConstFoo = 1; // Warning: should be marked as 'inline'
+
+   namespace N {
+ constexpr int NamespaceFoo = 1; // Warning: should be marked as 'inline'
+   }
+
+   extern const int ExternFoo; // Warning: should be converted to C++17 'inline variable'
+
+   struct S {
+ static const int StructFoo = 1; // OK: the variable is not at file scope
+   };
+
+   int NonConstFoo = 1; // No warning: non-const global variables have external linkage
+
+   const volatile int VolatileFoo = 1; // No warning: volatile global variables have external linkage
+
+   template
+   const auto TemplateFoo = sizeof(T); // OK: templates and their instantiations have external linkage
+
+   inline const int InlineFoo = 1; // no warning: already fixed
+
+   // No warning: C has no 'inline variables'
+   extern "C" {
+ const int CFoo0 = 1;
+   }
+   extern "C" const int CFoo1 = 1;
+
+   // No warning: 'inline' is invisible when within an unnamed namespace
+   namespace {
+ const int AnonNamespaceFoo = 1;
+   }
+
+Options
+---
+
+.. option:: HeaderFileExtensions
+
+   A comma-separated list of filename extensions of header files (the filename
+   extensions should not include "." prefix). Default is `h,hh,hpp,hxx`.
+   For header files without an extension, use an empty string (if there are no
+   other desired extensions) 

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp:64-68
+  unless(hasType(isVolatileQualified())), // non-volatile
+  unless(isTemplateVariable()),   // non-template
+  unless(isVarInline()),  // non-inline
+  unless(isExternC()),   // not "extern C" variable
+  unless(isInAnonymousNamespace())); // not within an anonymous namespace

LegalizeAdulthood wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood requested changes to this revision.
LegalizeAdulthood added a comment.
This revision now requires changes to proceed.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

In D118743#3290591 , 
@LegalizeAdulthood wrote:

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp:64-68
+  unless(hasType(isVolatileQualified())), // non-volatile
+  unless(isTemplateVariable()),   // non-template
+  unless(isVarInline()),  // non-inline
+  unless(isExternC()),   // not "extern C" variable
+  unless(isInAnonymousNamespace())); // not within an anonymous namespace

LegalizeAdulthood wrote:
> 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?
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.



Comment at: 
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h:19
+
+/// Finds non-extern non-inline function and variable definitions in header
+/// files, which can lead to potential ODR violations.

LegalizeAdulthood wrote:
> This block comment mentions functions, but the documentation only mentions 
> variables.
Oops, that's a typo: of course there are nothing abouth functions anywhere in 
the checker.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron updated this revision to Diff 416347.
Izaron marked 7 inline comments as done.
Izaron added a comment.
Herald added a project: All.

Fix issues


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-inline-const-variables-in-headers.hpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-inline-const-variables-in-headers.hpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-inline-const-variables-in-headers.hpp
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s -std=c++17 modernize-use-inline-const-variables-in-headers %t
+
+const int ConstFoo = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: global constant 'ConstFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
+// CHECK-FIXES: {{^}}inline const int ConstFoo = 1;{{$}}
+
+namespace N {
+constexpr int NamespaceFoo = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: global constant 'NamespaceFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
+// CHECK-FIXES: {{^}}inline constexpr int NamespaceFoo = 1;{{$}}
+} // namespace N
+
+extern const int ExternFoo;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: global constant 'ExternFoo' should be converted to C++17 'inline variable' [modernize-use-inline-const-variables-in-headers]
+
+struct S {
+  // no warning: the variable is not at file scope
+  static const int StructFoo = 1;
+};
+
+// no warning: non-const global variables have external linkage
+int NonConstFoo = 1;
+
+// no warning: volatile global variables have external linkage
+const volatile int VolatileFoo = 1;
+
+// no warning: templates and their instantiations have external linkage
+template 
+const auto TemplateFoo = sizeof(T);
+
+// no warning: already fixed
+inline const int InlineFoo = 1;
+
+// no warning: C has no 'inline variables'
+extern "C" {
+const int CFoo0 = 1;
+}
+extern "C" const int CFoo1 = 1;
+
+// no warning: 'inline' is invisible when within an unnamed namespace
+namespace {
+const int AnonNamespaceFoo = 1;
+} // namespace
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst
@@ -0,0 +1,69 @@
+.. title:: clang-tidy - modernize-use-inline-const-variables-in-headers
+
+modernize-use-inline-const-variables-in-headers
+===
+
+
+Suggests switching to C++17 ``inline variables`` for non-inline const
+variable definitions and extern const variable declarations in header files.
+Non-inline const variables make a separate instance of the variable for each
+translation unit that includes the header, which can lead to subtle violation
+of the ODR. Extern const variables are a deprecated way to define a constant
+since C++17.
+
+.. code-block:: c++
+
+   // Foo.h
+   const int ConstFoo = 1; // Warning: should be marked as 'inline'
+
+   namespace N {
+ constexpr int NamespaceFoo = 1; // Warning: should be marked as 'inline'
+   }
+
+   extern const int ExternFoo; // Warning: should be converted to C++17 'inline variable'
+
+   struct S {
+ static const int StructFoo = 1; // OK: the variable is not at file scope
+   };
+
+   int NonConstFoo = 1; // No warning: non-const global variables have external linkage
+
+   const volatile int VolatileFoo = 1; // No warning: volatile global variables have external linkage
+
+   template
+   const auto TemplateFoo = sizeof(T); // OK: templates and their instantiations have external linkage
+
+   inline const int InlineFoo = 1; // no warning: already fixed
+
+   // No warning: C has no 'inline variables'
+   extern "C" {
+ const int CFoo0 = 1;
+   }
+   extern "C" const int CFoo1 = 1;
+
+   // No warning: 'inline' is invisible when within an unnamed namespace
+   namespace {
+ const int AnonNamespaceFoo = 1;
+   }
+
+Options
+---
+
+.. option:: HeaderFileExtensions
+
+   A comma-separated list of filename extensions of header files (the filename
+   extensions should not include "." prefix). Default is `h,hh,hpp,hxx`.
+   For header files without an extension, use an empty string (if there are no
+   other desired extensions) or leave 

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp:64-68
+  unless(hasType(isVolatileQualified())), // non-volatile
+  unless(isTemplateVariable()),   // non-template
+  unless(isVarInline()),  // non-inline
+  unless(isExternC()),   // not "extern C" variable
+  unless(isInAnonymousNamespace())); // not within an anonymous namespace

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?



Comment at: 
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h:19
+
+/// Finds non-extern non-inline function and variable definitions in header
+/// files, which can lead to potential ODR violations.

This block comment mentions functions, but the documentation only mentions 
variables.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst:6
+
+Finds non-inline const variables definitions and extern const variables
+declarations in header in header files. Non-inline const variables make

Eugene.Zelenko wrote:
> Please synchronize first statement with Release Notes.
...`const variable definitions`...`const variable declarations`



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst:10
+includes the header, which can lead to subtle violation of the ODR.
+Extern const variables is a deprecated way to define a constant since C++17.
+

...`variables are a deprecated way`...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h:33
+/// declarations in header files. True by default.
+/// For the user-facing documentation see:
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.html

I think this is excessive comment. It's regular convention.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst:6
+
+Finds non-inline const variables definitions and extern const variables
+declarations in header in header files. Non-inline const variables make

Please synchronize first statement with Release Notes.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst:53
+   A comma-separated list of filename extensions of header files (the filename
+   extensions should not include "." prefix). Default is "h,hh,hpp,hxx".
+   For header files without an extension, use an empty string (if there are no

Please highlight option values in this section with single back-ticks. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron added a comment.

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 `izaronpl...@gmail.com`. Sorry for inconvenience!//


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118743/new/

https://reviews.llvm.org/D118743

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118743: [clang-tidy] Add `modernize-use-inline-const-variables-in-headers` check

[Evgeny Shulgin via Phabricator via cfe-commits]

Izaron created this revision.
Izaron added reviewers: alexfh, rsmith, LegalizeAdulthood.
Herald added subscribers: carlosgalvezp, xazax.hun, mgorny.
Izaron requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118743

Files:
  clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
  
clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseInlineConstVariablesInHeadersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst
  
clang-tools-extra/test/clang-tidy/checkers/modernize-use-inline-const-variables-in-headers.hpp

Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-inline-const-variables-in-headers.hpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-inline-const-variables-in-headers.hpp
@@ -0,0 +1,43 @@
+// RUN: %check_clang_tidy %s -std=c++17 modernize-use-inline-const-variables-in-headers %t
+
+const int ConstFoo = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: global constant 'ConstFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
+// CHECK-FIXES: {{^}}inline const int ConstFoo = 1;{{$}}
+
+namespace N {
+constexpr int NamespaceFoo = 1;
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: global constant 'NamespaceFoo' should be marked as 'inline' [modernize-use-inline-const-variables-in-headers]
+// CHECK-FIXES: {{^}}inline constexpr int NamespaceFoo = 1;{{$}}
+} // namespace N
+
+extern const int ExternFoo;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: global constant 'ExternFoo' should be converted to C++17 'inline variable' [modernize-use-inline-const-variables-in-headers]
+
+struct S {
+  // no warning: the variable is not at file scope
+  static const int StructFoo = 1;
+};
+
+// no warning: non-const global variables have external linkage
+int NonConstFoo = 1;
+
+// no warning: volatile global variables have external linkage
+const volatile int VolatileFoo = 1;
+
+// no warning: templates and their instantiations have external linkage
+template 
+const auto TemplateFoo = sizeof(T);
+
+// no warning: already fixed
+inline const int InlineFoo = 1;
+
+// no warning: C has no 'inline variables'
+extern "C" {
+const int CFoo0 = 1;
+}
+extern "C" const int CFoo1 = 1;
+
+// no warning: 'inline' is invisible when within an unnamed namespace
+namespace {
+const int AnonNamespaceFoo = 1;
+} // namespace
Index: clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/modernize-use-inline-const-variables-in-headers.rst
@@ -0,0 +1,67 @@
+.. title:: clang-tidy - modernize-use-inline-const-variables-in-headers
+
+modernize-use-inline-const-variables-in-headers
+===
+
+Finds non-inline const variables definitions and extern const variables
+declarations in header in header files. Non-inline const variables make
+a separate instance of the variable for each translation unit that
+includes the header, which can lead to subtle violation of the ODR.
+Extern const variables is a deprecated way to define a constant since C++17.
+
+.. code-block:: c++
+
+   // Foo.h
+   const int ConstFoo = 1; // Warning: should be marked as 'inline'
+
+   namespace N {
+ constexpr int NamespaceFoo = 1; // Warning: should be marked as 'inline'
+   }
+
+   extern const int ExternFoo; // Warning: should be converted to C++17 'inline variable'
+
+   struct S {
+ static const int StructFoo = 1; // OK: the variable is not at file scope
+   };
+
+   int NonConstFoo = 1; // No warning: non-const global variables have external linkage
+
+   const volatile int VolatileFoo = 1; // No warning: volatile global variables have external linkage
+
+   template
+   const auto TemplateFoo = sizeof(T); // OK: templates and their instantiations have external linkage
+
+   inline const int InlineFoo = 1; // no warning: already fixed
+
+   // No warning: C has no 'inline variables'
+   extern "C" {
+ const int CFoo0 = 1;
+   }
+   extern "C" const int CFoo1 = 1;
+
+   // No warning: 'inline' is invisible when within an unnamed namespace
+   namespace {
+ const int