[PATCH] D72378: [clang-tidy] Add `bugprone-reserved-identifier`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This check is missing a whole lot of reserved identifiers. For instance, in C++ it is missing everything from http://eel.is/c++draft/zombie.names and for C it is missing everything from future library directions. Are you intending to cover those cases as well?



================
Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:124
+    CheckFactories.registerCheck<ReservedIdentifierCheck>(
+        "bugprone-reserved-identifier");
     CheckFactories.registerCheck<SizeofContainerCheck>(
----------------
I think the CERT module should also be given an alias for this check, as it seems to cover: https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier and https://wiki.sei.cmu.edu/confluence/display/cplusplus/DCL51-CPP.+Do+not+declare+or+define+a+reserved+identifier


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:35
+
+static std::string collapseConsecutive(const StringRef str, const char c) {
+  std::string result;
----------------
Please drop top-level `const` qualifiers for parameters and locals -- that's not a style we typically use.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:48-51
+  if (hasDoubleUnderscore(Name)) {
+    Name.consume_front("__");
+    return collapseConsecutive(Name, '_');
+  }
----------------
Can't this still result in a reserved identifier? e.g.,
```
int ___Foobar; // three underscores
```


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:92
+  assert(!Name.empty());
+  if (std::find(Whitelist.begin(), Whitelist.end(), Name) != Whitelist.end())
+    return None;
----------------
`if (llvm::any_of(Whitelist, Name)) return None;`


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:98
+    Optional<FailureInfo> Info;
+    auto appendFailure = [&](StringRef Kind, std::string &&Fixup) {
+      if (!Info) {
----------------
`AppendFailure` per our usual naming conventions.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:106
+    };
+    auto inProgressFixup = [&] {
+      return Info
----------------
`InProgressFixup`


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:119-120
+
+    if (Info)
+      return Info;
+  } else {
----------------
You can just `return Info` here without the `if` statement -- the effect is the same.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h:21
+
+/// Checks for usages of identifiers reserved for use by the C++ implementation.
+/// The C++ standard reserves the following names for such use:
----------------
Why just C++? C has reserved identifiers as well, and there's a lot of overlap between the two languages in terms of what's reserved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72378