aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:216
+ {"long", "l"},
+ {"long long", "ll"},
+ {"unsigned long", "ul"},
----------------
`unsigned long long` -> `ull`
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:225
+ {"WORD", "w"},
+ {"DWORD", "dw"}};
+ // clang-format on
----------------
`ULONG` -> `ul`
`HANDLE` -> `h`
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:236
+ PrefixStr = "fn"; // Function Pointer
+ } else if (QT->isPointerType()) {
+ // clang-format off
----------------
I'm not certain how valid it is to look at just the type and decide that it's a
null-terminated string. For instance, the following is not an uncommon pattern:
`void something(const char *buffer, size_t length);` and it would be a bit
strange to change that into `szBuffer` as that would give an indication that
the buffer is null terminated. You could look at surrounding code for
additional information though, like other parameters in a function declaration.
As an aside, this sort of heuristic search may also allow you to change
`length` into `cbLength` instead of `nLength` for conventions like the
Microsoft one.
However, for local variables, I don't see how you could even come up with a
heuristic like you could with parameters, so I'm not certain what the right
answer is here.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:309
+
+ if (PtrCount > 0) {
+ for (size_t Idx = 0; Idx < PtrCount; Idx++) {
----------------
No need for this `if` statement, the `for` loop won't run anyway if `PtrCount
== 0`.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:319
+std::string
+IdentifierNamingCheck::getDeclTypeName(const clang::NamedDecl *Decl) const {
+ const ValueDecl *ValDecl = dyn_cast<ValueDecl>(Decl);
----------------
`ND` instead of `Decl`.
The function name doesn't really help me to understand why you'd call this as
opposed to getting the type information as a string from the `NamedDecl`
itself. I'm a bit worried about maintaining this code as the language evolves
-- Clang will get new keywords, and someone will have to remember to come
update this code. Could you get away with using
`Decl->getType()->getAsString()` and working with that rather than going back
to the original source code and trying to parse manually?
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:320
+IdentifierNamingCheck::getDeclTypeName(const clang::NamedDecl *Decl) const {
+ const ValueDecl *ValDecl = dyn_cast<ValueDecl>(Decl);
+ if (!ValDecl) {
----------------
`const auto *` since the type is spelled out in the initialization.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:554
+ case IdentifierNamingCheck::CT_HungarianNotation: {
+ const NamedDecl *ND = dyn_cast<NamedDecl>(InputDecl);
+ const std::string TypePrefix =
----------------
`const auto *` because the type is spelled out in the initialization.
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:39
+ std::string getDeclTypeName(const clang::NamedDecl *Decl) const;
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
----------------
Can you go with `ND` (or something else) instead of `Decl` since that's a type
name?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86671/new/
https://reviews.llvm.org/D86671
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
