[PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation

[Aaron Ballman via Phabricator via cfe-commits]

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