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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D72378#1812620 <https://reviews.llvm.org/D72378#1812620>, @logan-5 wrote:

> In D72378#1810763 <https://reviews.llvm.org/D72378#1810763>, @aaron.ballman wrote:
>
> > 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?
>
>
> I admit that those are outside the scope of what I had originally planned for this check -- I was primarily concerned about 'uglified' names, and writing a check that was 'invertible' in that regard. Now that you mention these, though, I do feel like this check doesn't live up to its name without including them.
>
> I'd be interested in incorporating them. It doesn't sound difficult, but it does sound like it'd be a sizable addition to this diff. Still familiarizing with the workflow around here... would it be alright to leave a TODO comment in this patch and add them in an upcoming patch, to keep this one more self-contained?


I would be fine putting a TODO in the code and addressing it in a follow-up patch. It's not critical for the initial offering.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:121
+    return Info;
+  } else {
+    if (!(hasReservedDoubleUnderscore(Name, LangOpts) ||
----------------
Sorry, I missed this before -- you can drop the `else` as well -- we don't use `else` after an unconditional `return` as part of our usual style guidelines.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:152-161
+      Failure.Info.KindName == "non-reserved"
+          ? "declaration uses identifier '%0', which is not a reserved "
+            "identifier"
+          : Failure.Info.KindName == "global-under"
+                ? "declaration uses identifier '%0', which is reserved in "
+                  "the global "
+                  "namespace; this causes undefined behavior"
----------------
Rather than use a complex conditional, I would prefer to see some string constants and a variable. It's a bit hard to reason about the diagnostic text as-is.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-reserved-identifier.rst:31
+namespace.
+
+Options
----------------
You should probably add some links here back to the CERT checks with some words like "this check also covers the following CERT secure coding guidelines" or somesuch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72378