[PATCH] D72213: [clang-tidy] Add `bugprone-reserved-identifier` to flag usages of reserved C++ names

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

I like this change, but I don't feel qualified to fully review the patch.

I wonder what happens if the project already contains a suggested fix, for example:

  #define __FOO(X) ...
  #define _FOO(X) __FOO(X)
  #define FOO(X) _FOO(X)

will it suggest:

  #define FOO(X) ...
  #define FOO(X) FOO(X)
  #define FOO(X) FOO(X)

?



================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:70
+                                      const bool IsInGlobalNamespace) {
+  return IsInGlobalNamespace && Name.size() > 1 && Name[0] == '_';
+}
----------------
`Name.size() > 1` doesn't catch `_` in the global namespace. Catching `_` will probably also cause issues with the automatic renaming.
What about renaming `__`?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp:21
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: declaration uses identifier 'Helper', which is not a reserved identifier [bugprone-reserved-identifier]
+// CHECK-FIXES: {{^}}struct __Helper {};{{$}}
+struct _helper2 {};
----------------
Why suggesting `__Helper` instead of  `_Helper` ?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier-invert.cpp:56
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: declaration uses identifier 'Up', which is not a reserved identifier [bugprone-reserved-identifier]
+// CHECK-FIXES: {{^}}template <class __Up>{{$}}
+inline reference_wrapper<const Up>
----------------
Likewise why not `_Up` ?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-reserved-identifier.cpp:68
+
+} // namespace __ns
+
----------------
Should it not suggest to change this to `namespace ns` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72213