aaron.ballman added a comment.
I think this patch is getting pretty close to finished -- have you tried
running it over a large corpus of code to see if it spots reserved identifiers
(or unreserved ones)? If not, I would recommend running it over LLVM + clang +
clang-tools-extra to see how it operates when looking for reserved identifiers
and libc++ for unreserved ones.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:27-34
+static const char NonReservedMessage[] =
+ "declaration uses identifier '%0', which is not a reserved "
+ "identifier";
+static const char GlobalUnderscoreMessage[] =
+ "declaration uses identifier '%0', which is reserved in the global "
+ "namespace; this causes undefined behavior";
+static const char DefaultMessage[] = "declaration uses reserved identifier "
----------------
I think you can reasonably combine these into a single diagnostic using
`%select`. e.g.,
`"declaration uses identifier %0, which is %select{a reserved identifier|not a
reserved identifier|reserved in the global namespace}1"`
I took out the bits about causing undefined behavior because that doesn't
really add much to the diagnostic (the "is a reserved identifier" bit should be
sufficient). Also, I removed the manual quoting around the `%0` because it
shouldn't be needed if you pass in a `NamedDecl*` as opposed to a string (which
you should prefer doing because it automatically formats the identifier
properly).
================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:65
+getDoubleUnderscoreFixup(StringRef Name, const LangOptions &LangOpts) {
+ if (hasReservedDoubleUnderscore(Name, LangOpts)) {
+ return collapseConsecutive(Name, '_');
----------------
Elide braces.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h:35
+ const bool Invert;
+ const std::vector<std::string> Whitelist;
+
----------------
Personal preference to name this `AllowedIdentifiers` or something along those
lines (and same for the user-facing option).
================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h:42
+
+protected:
+ llvm::Optional<FailureInfo>
----------------
This class is marked `final`, so why are these `protected` rather than
`private`?
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/bugprone-reserved-identifier.rst:8
+
+Checks for usages of identifiers reserved for use by the implementation.
+
----------------
You should mention that it currently does not check for all reserved names such
as future library or zombie names.
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
