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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 14 06:36:01 PST 2020


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





More information about the cfe-commits mailing list