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

[Logan Smith via Phabricator via cfe-commits]

logan-5 marked 2 inline comments as done.
logan-5 added a comment.

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?



================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:48-51
+  if (hasDoubleUnderscore(Name)) {
+    Name.consume_front("__");
+    return collapseConsecutive(Name, '_');
+  }
----------------
aaron.ballman wrote:
> Can't this still result in a reserved identifier? e.g.,
> ```
> int ___Foobar; // three underscores
> ```
`___Foobar` with 3 underscores will be fixed to `_Foobar` by this fixup, which is then passed through the underscore capital fixup, and that will be caught there. So it still works. 

Thinking about it more, though, I do think the `consume_front` is unnecessary. Without it, `__foo` would get changed (by this fixup) to `_foo`, which will be corrected by a later fixup if that's still invalid. If not, that's a smaller and less opinionated change to the name than the current `__foo` -> `foo`.

I think I'll take out the `consume_front("__")` and update the tests to match.


================
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:
----------------
aaron.ballman wrote:
> 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.
That's a good point. I'll do some tweaking to make sure this works well for C, including any places where C and C++ differ.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72378