aaron.ballman added a comment.
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?
================
Comment at: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp:124
+ CheckFactories.registerCheck<ReservedIdentifierCheck>(
+ "bugprone-reserved-identifier");
CheckFactories.registerCheck<SizeofContainerCheck>(
----------------
I think the CERT module should also be given an alias for this check, as it
seems to cover:
https://wiki.sei.cmu.edu/confluence/display/c/DCL37-C.+Do+not+declare+or+define+a+reserved+identifier
and
https://wiki.sei.cmu.edu/confluence/display/cplusplus/DCL51-CPP.+Do+not+declare+or+define+a+reserved+identifier
================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:35
+
+static std::string collapseConsecutive(const StringRef str, const char c) {
+ std::string result;
----------------
Please drop top-level `const` qualifiers for parameters and locals -- that's
not a style we typically use.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:48-51
+ if (hasDoubleUnderscore(Name)) {
+ Name.consume_front("__");
+ return collapseConsecutive(Name, '_');
+ }
----------------
Can't this still result in a reserved identifier? e.g.,
```
int ___Foobar; // three underscores
```
================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:92
+ assert(!Name.empty());
+ if (std::find(Whitelist.begin(), Whitelist.end(), Name) != Whitelist.end())
+ return None;
----------------
`if (llvm::any_of(Whitelist, Name)) return None;`
================
Comment at: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:98
+ Optional<FailureInfo> Info;
+ auto appendFailure = [&](StringRef Kind, std::string &&Fixup) {
+ if (!Info) {
----------------
`AppendFailure` per our usual naming conventions.
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:106
+ };
+ auto inProgressFixup = [&] {
+ return Info
----------------
`InProgressFixup`
================
Comment at:
clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp:119-120
+
+ if (Info)
+ return Info;
+ } else {
----------------
You can just `return Info` here without the `if` statement -- the effect is the
same.
================
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:
----------------
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.
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
