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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
