[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:23
 
-namespace {
-
-bool isCapsOnly(StringRef Name) {
-  return std::all_of(Name.begin(), Name.end(), [](const char C) {
-if (std::isupper(C) || std::isdigit(C) || C == '_')
-  return true;
-return false;
+static inline bool isCapsOnly(StringRef Name) {
+  return llvm::all_of(Name, [](const char C) {

LegalizeAdulthood wrote:
> carlosgalvezp wrote:
> > Nit: `inline` can be removed.
> Yeah, my IDE flagged it but since you asked for the `static`  `:)`
Sorry for the confusion, I should have added inline fixes directly to make it 
clear :) 



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:11
+`ES.31 
`_,
 and
+`ES.32 
`_.
+

LegalizeAdulthood wrote:
> carlosgalvezp wrote:
> > LegalizeAdulthood wrote:
> > > carlosgalvezp wrote:
> > > > Is ES.32 really checked by this check? I don't see any example or test 
> > > > that indicates that.
> > > > 
> > > > I'm also unsure if ES.32 should be handled here or via the 
> > > > "readability-identifier-naming" check, which is where you enforce a 
> > > > particular naming convention for different identifiers. Setting 
> > > > ALL_CAPS for macros there would be an effective way of solving ES.32.
> > > It was always handled through an option on this check.
> > > (Look at lines 49-56 of `MacroUsageCheck.cpp`)
> > > 
> > > It's a little bit odd, because it either checks for the names
> > > or it checks for the constant/function like macros, it never
> > > does both at the same time.
> > > 
> > > This is the way the check was originally written, I haven't
> > > changed any of that.
> > Ah I see it now! I got really confused by the `CheckCapsOnly` option. Not 
> > for this patch, but I think the following could be improved:
> > 
> > * Set it default to True, not False. People expect that check enforce a 
> > given guideline as good as possible by default. Options exist to deviate 
> > from the guidelines and relax them, which would be the case e.g. when 
> > introducing the check in an old codebase.
> > 
> > * Be renamed to something more descriptive and split it into 2 options with 
> > one single purpose. Right now this option controls a) enforcing ES.32 and 
> > b) applying warnings to macros with all caps or not.
> Yeah, I wasn't a fan of the way this option was influencing the behavior of 
> this check.
> 
> Can you open a github issue for the points you raised?
Absolutely, much better place for this!



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp:1
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t -- 
-header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later 
%t -- -header-filter=.* -system-headers --
 

LegalizeAdulthood wrote:
> carlosgalvezp wrote:
> > I'm curious as to why this is needed. If I remove it the test fails, on 
> > line 15, but the `u8` prefix was introduced already since C++11?
> The `u''` (UTF-16) and `U''` (UTF-32) character literals were added in C++11.
> The `u8''` (UTF-8) character literal was added in C++17.
> https://en.cppreference.com/w/cpp/language/character_literal
Duh, I was checking //string// literal, not //character// literal
https://en.cppreference.com/w/cpp/language/string_literal


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
LegalizeAdulthood marked an inline comment as done.
Closed by commit rGd83ecd77cc0f: [clang-tidy] Narrow cppguidelines-macro-usage 
to actual constants (authored by LegalizeAdulthood).

Changed prior to commit:
  https://reviews.llvm.org/D116386?vs=401242=401349#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116386

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t -- -header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later %t -- -header-filter=.* -system-headers --
 
 #ifndef INCLUDE_GUARD
 #define INCLUDE_GUARD
@@ -6,6 +6,21 @@
 #define PROBLEMATIC_CONSTANT 0
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant
 
+#define PROBLEMATIC_CONSTANT_CHAR '0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_WIDE_CHAR L'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_WIDE_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF8_CHAR u8'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF8_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF16_CHAR u'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF16_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF32_CHAR U'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF32_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
 #define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function
 
@@ -15,4 +30,17 @@
 #define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function
 
+// These are all examples of common macros that shouldn't have constexpr suggestions.
+#define COMMA ,
+
+#define NORETURN [[noreturn]]
+
+#define DEPRECATED attribute((deprecated))
+
+#if LIB_EXPORTS
+#define DLLEXPORTS __declspec(dllexport)
+#else
+#define DLLEXPORTS __declspec(dllimport)
+#endif
+
 #endif
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -7,10 +7,40 @@
 constructs exist for the task.
 
 The relevant sections in the C++ Core Guidelines are
-`Enum.1 `_,
-`ES.30 `_,
-`ES.31 `_ and
-`ES.33 `_.
+`ES.31 `_, and
+`ES.32 `_.
+
+Examples:
+
+.. code-block:: c++
+
+  #define C 0
+  #define F1(x, y) ((a) > (b) ? (a) : (b))
+  #define F2(...) (__VA_ARGS__)
+  #define COMMA ,
+  #define NORETURN [[noreturn]]
+  #define DEPRECATED attribute((deprecated))
+  #if LIB_EXPORTS
+  #define DLLEXPORTS __declspec(dllexport)
+  #else
+  #define DLLEXPORTS __declspec(dllimport)
+  #endif
+
+results in the following warnings:
+
+.. code-block:: c++
+
+  4 warnings generated.
+  test.cpp:1:9: warning: macro 'C' used to declare a constant; 

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 3 inline comments as done.
LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:11
+`ES.31 
`_,
 and
+`ES.32 
`_.
+

carlosgalvezp wrote:
> LegalizeAdulthood wrote:
> > carlosgalvezp wrote:
> > > Is ES.32 really checked by this check? I don't see any example or test 
> > > that indicates that.
> > > 
> > > I'm also unsure if ES.32 should be handled here or via the 
> > > "readability-identifier-naming" check, which is where you enforce a 
> > > particular naming convention for different identifiers. Setting ALL_CAPS 
> > > for macros there would be an effective way of solving ES.32.
> > It was always handled through an option on this check.
> > (Look at lines 49-56 of `MacroUsageCheck.cpp`)
> > 
> > It's a little bit odd, because it either checks for the names
> > or it checks for the constant/function like macros, it never
> > does both at the same time.
> > 
> > This is the way the check was originally written, I haven't
> > changed any of that.
> Ah I see it now! I got really confused by the `CheckCapsOnly` option. Not for 
> this patch, but I think the following could be improved:
> 
> * Set it default to True, not False. People expect that check enforce a given 
> guideline as good as possible by default. Options exist to deviate from the 
> guidelines and relax them, which would be the case e.g. when introducing the 
> check in an old codebase.
> 
> * Be renamed to something more descriptive and split it into 2 options with 
> one single purpose. Right now this option controls a) enforcing ES.32 and b) 
> applying warnings to macros with all caps or not.
Yeah, I wasn't a fan of the way this option was influencing the behavior of 
this check.

Can you open a github issue for the points you raised?


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:23
 
-namespace {
-
-bool isCapsOnly(StringRef Name) {
-  return std::all_of(Name.begin(), Name.end(), [](const char C) {
-if (std::isupper(C) || std::isdigit(C) || C == '_')
-  return true;
-return false;
+static inline bool isCapsOnly(StringRef Name) {
+  return llvm::all_of(Name, [](const char C) {

carlosgalvezp wrote:
> Nit: `inline` can be removed.
Yeah, my IDE flagged it but since you asked for the `static`  `:)`



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:99
+  if (!Message.empty())
+diag(MD->getLocation(), Message) << MacroName;
 }

carlosgalvezp wrote:
> Nit: tab with spaces.
Not sure how that tab got in there, LOL.
Probably my IDE "helping" me in an unwanted fashion.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp:1
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t -- 
-header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later 
%t -- -header-filter=.* -system-headers --
 

carlosgalvezp wrote:
> I'm curious as to why this is needed. If I remove it the test fails, on line 
> 15, but the `u8` prefix was introduced already since C++11?
The `u''` (UTF-16) and `U''` (UTF-32) character literals were added in C++11.
The `u8''` (UTF-8) character literal was added in C++17.
https://en.cppreference.com/w/cpp/language/character_literal


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:11
+`ES.31 
`_,
 and
+`ES.32 
`_.
+

LegalizeAdulthood wrote:
> carlosgalvezp wrote:
> > Is ES.32 really checked by this check? I don't see any example or test that 
> > indicates that.
> > 
> > I'm also unsure if ES.32 should be handled here or via the 
> > "readability-identifier-naming" check, which is where you enforce a 
> > particular naming convention for different identifiers. Setting ALL_CAPS 
> > for macros there would be an effective way of solving ES.32.
> It was always handled through an option on this check.
> (Look at lines 49-56 of `MacroUsageCheck.cpp`)
> 
> It's a little bit odd, because it either checks for the names
> or it checks for the constant/function like macros, it never
> does both at the same time.
> 
> This is the way the check was originally written, I haven't
> changed any of that.
Ah I see it now! I got really confused by the `CheckCapsOnly` option. Not for 
this patch, but I think the following could be improved:

* Set it default to True, not False. People expect that check enforce a given 
guideline as good as possible by default. Options exist to deviate from the 
guidelines and relax them, which would be the case e.g. when introducing the 
check in an old codebase.

* Be renamed to something more descriptive and split it into 2 options with one 
single purpose. Right now this option controls a) enforcing ES.32 and b) 
applying warnings to macros with all caps or not.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp accepted this revision.
carlosgalvezp added a comment.
This revision is now accepted and ready to land.

Looks reasonable to me! I'm fairly new here so I might not be the most 
"authoritative reviewer", let me know if you'd like someone else to give the 
final approval




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:23
 
-namespace {
-
-bool isCapsOnly(StringRef Name) {
-  return std::all_of(Name.begin(), Name.end(), [](const char C) {
-if (std::isupper(C) || std::isdigit(C) || C == '_')
-  return true;
-return false;
+static inline bool isCapsOnly(StringRef Name) {
+  return llvm::all_of(Name, [](const char C) {

Nit: `inline` can be removed.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:99
+  if (!Message.empty())
+diag(MD->getLocation(), Message) << MacroName;
 }

Nit: tab with spaces.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp:1
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t -- 
-header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later 
%t -- -header-filter=.* -system-headers --
 

I'm curious as to why this is needed. If I remove it the test fails, on line 
15, but the `u8` prefix was introduced already since C++11?


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 401242.
LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.

- Update from comments


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

https://reviews.llvm.org/D116386

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t -- -header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later %t -- -header-filter=.* -system-headers --
 
 #ifndef INCLUDE_GUARD
 #define INCLUDE_GUARD
@@ -6,6 +6,21 @@
 #define PROBLEMATIC_CONSTANT 0
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant
 
+#define PROBLEMATIC_CONSTANT_CHAR '0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_WIDE_CHAR L'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_WIDE_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF8_CHAR u8'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF8_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF16_CHAR u'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF16_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF32_CHAR U'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF32_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
 #define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function
 
@@ -15,4 +30,17 @@
 #define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function
 
+// These are all examples of common macros that shouldn't have constexpr suggestions.
+#define COMMA ,
+
+#define NORETURN [[noreturn]]
+
+#define DEPRECATED attribute((deprecated))
+
+#if LIB_EXPORTS
+#define DLLEXPORTS __declspec(dllexport)
+#else
+#define DLLEXPORTS __declspec(dllimport)
+#endif
+
 #endif
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -7,10 +7,40 @@
 constructs exist for the task.
 
 The relevant sections in the C++ Core Guidelines are
-`Enum.1 `_,
-`ES.30 `_,
-`ES.31 `_ and
-`ES.33 `_.
+`ES.31 `_, and
+`ES.32 `_.
+
+Examples:
+
+.. code-block:: c++
+
+  #define C 0
+  #define F1(x, y) ((a) > (b) ? (a) : (b))
+  #define F2(...) (__VA_ARGS__)
+  #define COMMA ,
+  #define NORETURN [[noreturn]]
+  #define DEPRECATED attribute((deprecated))
+  #if LIB_EXPORTS
+  #define DLLEXPORTS __declspec(dllexport)
+  #else
+  #define DLLEXPORTS __declspec(dllimport)
+  #endif
+
+results in the following warnings:
+
+.. code-block:: c++
+
+  4 warnings generated.
+  test.cpp:1:9: warning: macro 'C' used to declare a constant; consider using a 'constexpr' constant [cppcoreguidelines-macro-usage]
+  #define C 0
+  ^
+  test.cpp:2:9: warning: function-like macro 'F1' used; consider a 'constexpr' template function 

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:11
+`ES.31 
`_,
 and
+`ES.32 
`_.
+

carlosgalvezp wrote:
> Is ES.32 really checked by this check? I don't see any example or test that 
> indicates that.
> 
> I'm also unsure if ES.32 should be handled here or via the 
> "readability-identifier-naming" check, which is where you enforce a 
> particular naming convention for different identifiers. Setting ALL_CAPS for 
> macros there would be an effective way of solving ES.32.
It was always handled through an option on this check.
(Look at lines 49-56 of `MacroUsageCheck.cpp`)

It's a little bit odd, because it either checks for the names
or it checks for the constant/function like macros, it never
does both at the same time.

This is the way the check was originally written, I haven't
changed any of that.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:23
 
-namespace {
-
-bool isCapsOnly(StringRef Name) {
-  return std::all_of(Name.begin(), Name.end(), [](const char C) {
-if (std::isupper(C) || std::isdigit(C) || C == '_')
-  return true;
-return false;
+inline bool isCapsOnly(StringRef Name) {
+  return llvm::all_of(Name, [](const char C) {

Shouldn't it still be "static"? "inline" will not give internal linkage. I 
think it's also unnecessary in this case since there's no risk for multiple 
definitions (it's not defined in a header file)



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:11
+`ES.31 
`_,
 and
+`ES.32 
`_.
+

Is ES.32 really checked by this check? I don't see any example or test that 
indicates that.

I'm also unsure if ES.32 should be handled here or via the 
"readability-identifier-naming" check, which is where you enforce a particular 
naming convention for different identifiers. Setting ALL_CAPS for macros there 
would be an effective way of solving ES.32.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 401087.
LegalizeAdulthood added a comment.

- For function `isCapsOnly`:
  - Declare as static per LLVM style guide
  - Simplify boolean expression
  - Use `llvm::all_of` on container
- Inline Function `isLiteralTokenSequence` as it is now simpler with 
`llvm::all_of`
- Update documentation:
  - Remove references to core guidelines that aren't implemented
  - Add mention of reduced false positives in the release notes
  - Give example in check documentation


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

https://reviews.llvm.org/D116386

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t -- -header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later %t -- -header-filter=.* -system-headers --
 
 #ifndef INCLUDE_GUARD
 #define INCLUDE_GUARD
@@ -6,6 +6,21 @@
 #define PROBLEMATIC_CONSTANT 0
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant
 
+#define PROBLEMATIC_CONSTANT_CHAR '0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_WIDE_CHAR L'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_WIDE_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF8_CHAR u8'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF8_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF16_CHAR u'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF16_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF32_CHAR U'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF32_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
 #define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function
 
@@ -15,4 +30,17 @@
 #define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function
 
+// These are all examples of common macros that shouldn't have constexpr suggestions.
+#define COMMA ,
+
+#define NORETURN [[noreturn]]
+
+#define DEPRECATED attribute((deprecated))
+
+#if LIB_EXPORTS
+#define DLLEXPORTS __declspec(dllexport)
+#else
+#define DLLEXPORTS __declspec(dllimport)
+#endif
+
 #endif
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -7,10 +7,40 @@
 constructs exist for the task.
 
 The relevant sections in the C++ Core Guidelines are
-`Enum.1 `_,
-`ES.30 `_,
-`ES.31 `_ and
-`ES.33 `_.
+`ES.31 `_, and
+`ES.32 `_.
+
+Examples:
+
+.. code-block:: c++
+
+  #define C 0
+  #define F1(x, y) ((a) > (b) ? (a) : (b))
+  #define F2(...) (__VA_ARGS__)
+  #define COMMA ,
+  #define NORETURN [[noreturn]]
+  #define DEPRECATED attribute((deprecated))
+  #if LIB_EXPORTS
+  #define DLLEXPORTS __declspec(dllexport)
+  #else
+  #define DLLEXPORTS __declspec(dllimport)
+  #endif
+
+results in the 

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D116386#3252351 , @carlosgalvezp 
wrote:

> Would it be worth mentioning the change in the release notes? [...]

Updating the documentation based on the discussion thread and
the release notes are both good ideas, I'll do that and post an update.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:85
+
+bool isLiteralTokenSequence(const MacroInfo *Info) {
+  return std::all_of(Info->tokens_begin(), Info->tokens_end(),

LLVM coding standards say that this function should be "static", keeping 
anonymous namespaces only to classes 


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Code looks great! Would it be worth mentioning the change in the release notes? 
I also wonder if the check documentation needs some updates. From what i read 
in the discussion this rule has poor enforcement in the guidelines so perhaps 
it's good to clarify what exactly the check covers. I'm afk right now so i 
can't check this in detail :)


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Ping again.  This change isn't that long or complicated and fixes
a bug that results in false positives from this check.

Please give it a review.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Ping.

This review has been waiting for a week without any comment.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 399186.
LegalizeAdulthood added a comment.

- clang-format
- use Token::isLiteral instead of homebrew token predicate
- rename predicate on token list to reflect isLiteral


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

https://reviews.llvm.org/D116386

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t -- -header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later %t -- -header-filter=.* -system-headers --
 
 #ifndef INCLUDE_GUARD
 #define INCLUDE_GUARD
@@ -6,6 +6,21 @@
 #define PROBLEMATIC_CONSTANT 0
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant
 
+#define PROBLEMATIC_CONSTANT_CHAR '0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_WIDE_CHAR L'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_WIDE_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF8_CHAR u8'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF8_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF16_CHAR u'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF16_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF32_CHAR U'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF32_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
 #define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function
 
@@ -15,4 +30,17 @@
 #define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function
 
+// These are all examples of common macros that shouldn't have constexpr suggestions.
+#define COMMA ,
+
+#define NORETURN [[noreturn]]
+
+#define DEPRECATED attribute((deprecated))
+
+#if LIB_EXPORTS
+#define DLLEXPORTS __declspec(dllexport)
+#else
+#define DLLEXPORTS __declspec(dllimport)
+#endif
+
 #endif
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -14,6 +14,7 @@
 #include "llvm/Support/Regex.h"
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace tidy {
@@ -32,7 +33,8 @@
 class MacroUsageCallbacks : public PPCallbacks {
 public:
   MacroUsageCallbacks(MacroUsageCheck *Check, const SourceManager ,
-  StringRef RegExpStr, bool CapsOnly, bool IgnoreCommandLine)
+  StringRef RegExpStr, bool CapsOnly,
+  bool IgnoreCommandLine)
   : Check(Check), SM(SM), RegExp(RegExpStr), CheckCapsOnly(CapsOnly),
 IgnoreCommandLineMacros(IgnoreCommandLine) {}
   void MacroDefined(const Token ,
@@ -78,22 +80,34 @@
   this, SM, AllowedRegexp, CheckCapsOnly, IgnoreCommandLineMacros));
 }
 
+namespace {
+
+bool isLiteralTokenSequence(const MacroInfo *Info) {
+  return std::all_of(Info->tokens_begin(), Info->tokens_end(),
+ std::mem_fn(::isLiteral));
+}
+
+} // namespace
+
 void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) {
-  StringRef Message =
-  "macro '%0' used to declare a constant; consider using a 'constexpr' "
-  "constant";
+  const MacroInfo *Info = MD->getMacroInfo();
+  StringRef Message;
 
+  if (isLiteralTokenSequence(Info))
+Message = "macro '%0' used to declare a constant; consider using a "
+  "'constexpr' constant";
   /// A variadic macro is function-like at the same time. Therefore variadic
   /// macros are checked first and will be excluded for the function-like
   /// diagnostic.
-  if (MD->getMacroInfo()->isVariadic())
+  else if (Info->isVariadic())
 Message = "variadic macro '%0' used; consider using a 'constexpr' "
   "variadic template function";
-  else if 

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a reviewer: JonasToth.
LegalizeAdulthood added a comment.

I'm adding the original author of this check as a reviewer.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D116386#3227236 , @aaron.ballman 
wrote:

> We're in strong agreement that guideline checkers with untenable false 
> positive rates are a
> very bad thing. I think this particular check is of insufficient quality to 
> have been added in
> the first place because it's based on a set of rules that are not generally 
> enforceable in a
> way that isn't a constantly nagging for reasonable real world code.

I think this guideline is mostly what this check is trying to accomplish:
ES.31: Don't use macros for constants or "functions"

Now, for the first part about constants, the check was issuing way too many
false positives.  That's what I fix in this review: it now only suggests that 
the
macro be replaced with a `constexpr` constant when the macro expansion is
truly a constant.

Ironically I was working on my own check that covers some aspects of 
"Enum.1: Prefer enumerations over macros 
".
However, there isn't any way an automated tool could recognize the example
given in the guidelines because the macros in question don't share a common
prefix, which I've found is a good heuristic for enums disguised as macros.
There are additional heuristics one could apply, such as when a bunch of
constant macros are defined on successive lines.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D116386#3226174 , 
@LegalizeAdulthood wrote:

> Aaron, I think your comments are useful and I would be inclined to agree with 
> you if I
> was the original author of this check.

Sorry, I hope I didn't give the impression I thought you had done anything 
wrong here, because you definitely haven't. I appreciate all of your efforts 
(and am glad to see you back in the community), and my resigning from the 
review is not indicative of me not wanting to collaborate with you! It's more a 
matter of: I've got ~50 code reviews in my queue as of this morning, on top of 
my own work, and I didn't want you to have to ping this for months on end only 
to never get feedback from me because C++ Core Guideline reviews always stay at 
the bottom of my queue due to the amount of effort involved in most of them.

> I treat the guidelines as just that: guidelines,
> not rules.  In the context of clang-tidy I think you're correct that some 
> guidelines
> are easily turned into usable diagnostics and a subset of those can become 
> enforceable
> rules with suggested fixits.
>
> In this case, the check only issues diagnostics, not fixits.  When the 
> diagnostics result
> in many false positives (as per the open bug), then I think it's reasonable 
> to narrow the
> scope of the check to omit the false positives.

This is not how clang-tidy typically handles coding guideline-specific rules. 
The general rule in clang-tidy is for checks based on coding guidelines to be 
configured to follow the guideline by default (with options to help tune 
things). If of sufficient value, we will add an alias check in `bugprone` (or 
another module that makes sense) and give it different default configuration 
settings than the guideline check, but users expect something that claims to 
check a guideline to actually check that guideline.

(The alternative is: we go back to the guideline authors and ask them to please 
improve their guidance and then we implement whatever the new rule says. But 
the end result is the same -- for checks in a module specific to a published 
set of guidelines, deviations from the spec are considered bugs to be fixed 
, and the guideline text is the final arbiter of what the correct 
behavior is.)

> The worst thing a "guideline" checker can do is to constantly nag you about 
> false
> positives.  This trains people to not run the checkers and/or ignore all 
> their complaints.

We're in strong agreement that guideline checkers with untenable false positive 
rates are a very bad thing. I think this particular check is of insufficient 
quality to have been added in the first place because it's based on a set of 
rules that are not generally enforceable in a way that isn't a constantly 
nagging for reasonable real world code.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Also, re-reading the C++ guidelines, I agree that this check is handling the 
two cases
ES.31 (Don't use macros for constants and functions)
ES.33 (Don't use macros that aren't all caps)

I guess previously it **technically** handled ES.30 (don't use macros for text 
manipulation)
by just complaining about every macro not being replaced with a `constexpr` 
constant,
LOL.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Aaron, I think your comments are useful and I would be inclined to agree with 
you if I
was the original author of this check.  I treat the guidelines as just that: 
guidelines,
not rules.  In the context of clang-tidy I think you're correct that some 
guidelines
are easily turned into usable diagnostics and a subset of those can become 
enforceable
rules with suggested fixits.

In this case, the check only issues diagnostics, not fixits.  When the 
diagnostics result
in many false positives (as per the open bug), then I think it's reasonable to 
narrow the
scope of the check to omit the false positives.

The worst thing a "guideline" checker can do is to constantly nag you about 
false
positives.  This trains people to not run the checkers and/or ignore all their 
complaints.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman edited reviewers, added: hokein, whisperity, njames93; removed: 
aaron.ballman.
aaron.ballman added a comment.
Herald added a subscriber: rnkovacs.

We run into this problem quite frequently -- the C++ Core Guidelines put very 
little effort into thinking about enforcement, and so tool vendors like us are 
left holding the bag. We can either do what the rule says for enforcement 
(which is what users often expect to happen, because the rules are supposed to 
be the source of truth), or we can do something actually useful. For example, 
this rule is documented 
(https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-macro-usage.html)
 to cover ES.30 and ES.31 whose enforcement is `Scream when you see a macro 
that isn't just used for source control (e.g., #ifdef)`. When the rule and the 
check disagree, we usually ask check authors to work with the guideline authors 
to come to an agreement on what the rule should be changed to say and I think 
we need to do that dance again here. (Aside: our docs also claim this checks 
for ES.33 whose enforcement is `Warn against short macro names.` and I don't 
see anything in the check that actually does that; I think the docs meant ES.32 
about defining macro names in all caps, and the option for this part of the 
check is not enabled by default.)

However, my personal experience on engaging with the C++ Core Guidelines 
authors on enforcement issues in the past has not been positive, which is why I 
made a personal decision to not review C++ Core Guidelines for clang-tidy once 
they started hitting enforcement issues where the rule is of too low quality (I 
don't have the time to do that amount of work on behalf of the C++ Core 
Guidelines). I think that's the case here, so I'm resigning from the review. 
However, if the C++ Core Guidelines authors engage in the topic and improve 
their enforcement to be usefully enforceable, I'm happy to be added back as a 
reviewer.

I'm adding a few new reviewers who might have more ability to engage on the 
review in the near term.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

Can someone look at this review please, it's not that long and/or complicated.  
Thanks.


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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood updated this revision to Diff 396683.
LegalizeAdulthood marked an inline comment as not done.
LegalizeAdulthood added a comment.

Improve name of predicate function.
Use standard algorithms instead of raw loops


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

https://reviews.llvm.org/D116386

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t -- -header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later %t -- -header-filter=.* -system-headers --
 
 #ifndef INCLUDE_GUARD
 #define INCLUDE_GUARD
@@ -6,6 +6,21 @@
 #define PROBLEMATIC_CONSTANT 0
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant
 
+#define PROBLEMATIC_CONSTANT_CHAR '0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_WIDE_CHAR L'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_WIDE_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF8_CHAR u8'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF8_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF16_CHAR u'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF16_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF32_CHAR U'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF32_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
 #define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function
 
@@ -15,4 +30,17 @@
 #define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function
 
+// These are all examples of common macros that shouldn't have constexpr suggestions.
+#define COMMA ,
+
+#define NORETURN [[noreturn]]
+
+#define DEPRECATED attribute((deprecated))
+
+#if LIB_EXPORTS
+#define DLLEXPORTS __declspec(dllexport)
+#else
+#define DLLEXPORTS __declspec(dllimport)
+#endif
+
 #endif
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -78,22 +78,53 @@
   this, SM, AllowedRegexp, CheckCapsOnly, IgnoreCommandLineMacros));
 }
 
+namespace {
+
+bool isConstantTokenSequence(const MacroInfo *Info) {
+  auto IsConstantToken = [](const Token ) {
+switch (Token.getKind()) {
+case tok::comment:
+case tok::numeric_constant:
+case tok::char_constant:
+case tok::wide_char_constant:
+case tok::utf8_char_constant:
+case tok::utf16_char_constant:
+case tok::utf32_char_constant:
+case tok::string_literal:
+case tok::wide_string_literal:
+case tok::header_name:
+case tok::utf8_string_literal:
+case tok::utf16_string_literal:
+case tok::utf32_string_literal:
+  return true;
+default:
+  return false;
+}
+  };
+  return std::all_of(Info->tokens_begin(), Info->tokens_end(), IsConstantToken);
+}
+
+} // namespace
+
 void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) {
-  StringRef Message =
-  "macro '%0' used to declare a constant; consider using a 'constexpr' "
-  "constant";
+  const MacroInfo *Info = MD->getMacroInfo();
+  StringRef Message;
 
+  if (isConstantTokenSequence(Info))
+Message = "macro '%0' used to declare a constant; consider using a "
+  "'constexpr' constant";
   /// A variadic macro is function-like at the same time. Therefore variadic
   /// macros are checked first and will be excluded for the function-like
   /// diagnostic.
-  if (MD->getMacroInfo()->isVariadic())
+  else if (Info->isVariadic())
 Message = "variadic macro '%0' used; consider using a 'constexpr' "
   "variadic template function";
-  else if (MD->getMacroInfo()->isFunctionLike())
+  else if (Info->isFunctionLike())
 

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:83-105
+bool isConstantToken(const MacroDirective *MD) {
+  for (const auto  : MD->getMacroInfo()->tokens()) {
+switch (Token.getKind()) {
+case tok::comment:
+case tok::numeric_constant:
+case tok::char_constant:
+case tok::wide_char_constant:

In the spirit of "avoid raw loops", would this be clearer expressed as 
std::all_of and the switch/case provided as a lambda?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116386

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

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood created this revision.
LegalizeAdulthood added a reviewer: alexfh.
Herald added subscribers: carlosgalvezp, kbarton, xazax.hun, nemanjai.
LegalizeAdulthood requested review of this revision.
Herald added a project: clang-tools-extra.

Previously, any macro that didn't look like a varargs macro
or a function style macro was reported with a warning that
it should be replaced with a constexpr const declaration.
This is only reasonable when the macro body contains constants
and not expansions like ",", "[[noreturn]]", "__declspec(xxx)",
etc.

So instead of always issuing a warning about every macro that
doesn't look like a varargs or function style macro, examine the
tokens in the macro and only warn about the macro if it contains
only comment and constant tokens.

Fixes #39945


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116386

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-macro-usage.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t -- -header-filter=.* -system-headers --
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage -std=c++17-or-later %t -- -header-filter=.* -system-headers --
 
 #ifndef INCLUDE_GUARD
 #define INCLUDE_GUARD
@@ -6,6 +6,21 @@
 #define PROBLEMATIC_CONSTANT 0
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT' used to declare a constant; consider using a 'constexpr' constant
 
+#define PROBLEMATIC_CONSTANT_CHAR '0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_WIDE_CHAR L'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_WIDE_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF8_CHAR u8'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF8_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF16_CHAR u'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF16_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_CONSTANT_UTF32_CHAR U'0'
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro 'PROBLEMATIC_CONSTANT_UTF32_CHAR' used to declare a constant; consider using a 'constexpr' constant
+
 #define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro 'PROBLEMATIC_FUNCTION' used; consider a 'constexpr' template function
 
@@ -15,4 +30,17 @@
 #define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
 // CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro 'PROBLEMATIC_VARIADIC2' used; consider using a 'constexpr' variadic template function
 
+// These are all examples of common macros that shouldn't have constexpr suggestions.
+#define COMMA ,
+
+#define NORETURN [[noreturn]]
+
+#define DEPRECATED attribute((deprecated))
+
+#if LIB_EXPORTS
+#define DLLEXPORTS __declspec(dllexport)
+#else
+#define DLLEXPORTS __declspec(dllimport)
+#endif
+
 #endif
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -78,22 +78,52 @@
   this, SM, AllowedRegexp, CheckCapsOnly, IgnoreCommandLineMacros));
 }
 
+namespace {
+
+bool isConstantToken(const MacroDirective *MD) {
+  for (const auto  : MD->getMacroInfo()->tokens()) {
+switch (Token.getKind()) {
+case tok::comment:
+case tok::numeric_constant:
+case tok::char_constant:
+case tok::wide_char_constant:
+case tok::utf8_char_constant:
+case tok::utf16_char_constant:
+case tok::utf32_char_constant:
+case tok::string_literal:
+case tok::wide_string_literal:
+case tok::header_name:
+case tok::utf8_string_literal:
+case tok::utf16_string_literal:
+case tok::utf32_string_literal:
+  break;
+default:
+  return false;
+}
+  }
+  return true;
+}
+
+} // namespace
+
 void MacroUsageCheck::warnMacro(const MacroDirective *MD, StringRef MacroName) {
-  StringRef Message =
-  "macro '%0' used to declare a constant; consider using a 'constexpr' "
-  "constant";
+  StringRef Message;
 
+  if (isConstantToken(MD))
+Message = "macro '%0' used to declare a constant; consider using a "
+  "'constexpr' constant";
   /// A variadic macro is