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