Based on CERT-DCL51-CPP
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
docs/clang-tidy/checks/cert-dcl51-cpp.rst | ||
---|---|---|
34 | Will be good idea to mention that this is Clang warnings. Please also make consistent indentation for lists here and above. |
clang-tidy/cert/AvoidReservedNamesCheck.cpp | ||
---|---|---|
23 | Is this clang formatted? It will be probably better to have |
clang-tidy/cert/AvoidReservedNamesCheck.cpp | ||
---|---|---|
23 | I'm a bit worried about this list getting out of sync with the frontend. @alexfh, do you have any reservations about using TokenKinds.def from the frontend to generate this list? | |
130 | Might as well not name the MacroDirective argument here and below, since it's unused. | |
142 | You can use isInvalid() here instead of negating isValid(). | |
153–154 | I am a bit concerned about the performance of this check -- there are bound to be a *lot* of named declarations in a source file. Performance numbers would be helpful. | |
163 | isInvalid() | |
177 | How about: "identifiers containing double underscores are reserved to the implementation" | |
179 | Similarly: "names declared at global scope that start with an underscore are reserved to the implementation" | |
181 | Similarly: "identifiers that begin with an underscore followed by an uppercase letter are reserved to the implementation" | |
189 | const auto * | |
201 | Instead of using the plural form, I'd use singular: "do not use a macro name that is identical to a keyword or an attribute" Note, I think we want to have an escape hatch for macro names that are identical to a keyword when the replacement list is is the name of the keyword with underscores removed. e.g., #define inline __inline #define const const | |
docs/clang-tidy/checks/cert-dcl51-cpp.rst | ||
7–8 | Perhaps: This check will catch the following errors due to the names being reserved to the implementation: | |
test/clang-tidy/Inputs/Headers/sha.h | ||
1 ↗ | (On Diff #101200) | I'm not keen on the name of this header file. Can you rename it something more like "dcl51.h"? |
clang-tidy/cert/AvoidReservedNamesCheck.cpp | ||
---|---|---|
23 | If I'm not missing anything, this should be pretty straightforward for the names listed there. However, things like optimize_for_synchronized are not yet mentioned in TokenKinds.def. Options are to add them there or extend the list in the check. |
clang-tidy/cert/AvoidReservedNamesCheck.cpp | ||
---|---|---|
23 | Thanks for the advice, I changed the list to use TokenKinds.def. However, I'm not really aware what you mean by the "thing like optimize_for_synchronized ". Where can I find the others that are missing and what are these? | |
153–154 | I checked the performance with two configurations. First like this: -checks=-*,cert-dcl51-cpp,modernize-unary-static-assert The result: real 49m53.389s user 290m45.508s sys 7m45.504s Then like this: -checks=-*,modernize-unary-static-assert The result: real 48m45.531s user 284m33.860s sys 7m33.480s |
clang-tidy/cert/AvoidReservedNamesCheck.cpp | ||
---|---|---|
23 |
Well, the hard-coded list in the first version of your patch included "optimize_for_synchronized". I have no idea what is that (google says it's from the transactional memory TS) and I also don't know if any other entries from that original list are missing from TokenKinds.def. You might want to dump the list from the new version of your patch and compare it with the old one. If something is missing, it makes sense adding it to the list (either here or maybe even to TokenKinds.def, if nobody has concerns about that). | |
58–59 | How about a range for loop here? | |
60–63 | I'd avoid creating new strings and use something like this instead: x.endswith(y) && (x.length() == y.length() || (x.length() == y.length() + 2 && x.startswith("__"))). |
clang-tidy/cert/AvoidReservedNamesCheck.cpp | ||
---|---|---|
23 | I added the attribute specifier sequence attributes to the first list since the standard highlighted to avoid attribute tokens: "A translation unit shall not #define or #undef names lexically identical to keywords, to the identifiers listed in Table 3, or to the attribute-tokens described in 7.6.". I'm not sure if these belong to TokenKinds.def or there's a reason why these have been left out, so I just listed them here. |
clang-tidy/cert/AvoidReservedNamesCheck.cpp | ||
---|---|---|
60–63 | This condition is not equivalent to the one you had before. It doesn't ensure the prefix is "__". BTW, should the check complain on all names starting with __? [global.names] says: |
clang-tidy/cert/AvoidReservedNamesCheck.cpp | ||
---|---|---|
60–63 | You're right, the check should warn for global names starting with "__" but this part is based on the exception: // Allowed compilers with semantically equivalent extension behaviour |
clang-tidy/cert/AvoidReservedNamesCheck.cpp | ||
---|---|---|
23 | I'd use StringSet<> for faster lookups and maybe also wrap this in a function: static bool IsKeyword(StringRef Name) { const static StringSet<> Keywords = { ... }; return Keywords.count(Name) != 0; } | |
69–76 | The code is more confusing than it could be. This also doesn't do what it claims to do. IIUC, it will accept macro definitions like #define const const const const. Instead, I'd suggest a less "clever" code: const auto &Tokens = MD->getMacroInfo()->tokens(); return Tokens.size() == 1 && identical(MNT.getIdentifierInfo()->getName(), Tokens.front().getIdentifierInfo()->getName()); | |
79 | Please add a comment about this exception, maybe with an example of what it allows. | |
109 | nit: Function names are better when they are verbs, e.g. here checkDeclNameIsReserved would be clearer, IMHO. Same applies to macroNameIsKeywordCheck below and macroNameCheck above. | |
133 | It's trivial to make this function iterative without sacrificing readability: const DeclContext *DC = D->getDeclContext(); while (const auto *NS = dyn_cast<NamespaceDecl>(DC)) { if (!NS->isAnonymousNamespace()) return false; DC = NS->getDeclContext(); } return DC->isTranslationUnit(); | |
clang-tidy/cert/AvoidReservedNamesCheck.h | ||
33 | nit: Please add an empty line above (or just clang-format the code). | |
test/clangd/definitions.test | ||
141 ↗ | (On Diff #108031) | Unrelated change? |
clang-tidy/cert/AvoidReservedNamesCheck.cpp | ||
---|---|---|
23 | While it's not incorrect to list the attribute names, it's also a bit inconsistent to do it. C++17 defines a list of zombie names, there's also "A translation unit that includes a standard library header shall not #define or #undef names declared in any standard library header.", etc. It's a bit strange that attributes get more special treatment than names declared in standard library headers. However, I'm not opposed to listing the attribute names if you think this is better. I think we should just drop non-standard attributes (including ones from TSes), like optimize_for_synchronized, especially when our implementation does not implement those attributes. |
clang-tidy/cert/AvoidReservedNamesCheck.cpp | ||
---|---|---|
63–67 | There's a more common and less tautological equivalent for if (X) return true; return false It's return X;. | |
79 |
| |
96–99 | I'd suggest restructuring the code to avoid an unnecessary lookup (and make it clear both "var" and "label" aren't expected), e.g.: const auto *ND = Result.Nodes.getNodeAs<NamedDecl>("var"); if (!ND) { if (const auto *L = Result.Nodes.getNodeAs<LabelStmt>("label")) ND = L->getDecl(); } | |
docs/clang-tidy/checks/cert-dcl51-cpp.rst | ||
31 | s/Clang checks/Clang diagnostics/ |
nit: Please add an empty line above (or just clang-format the code).