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

[Douglas Chen via Phabricator via cfe-commits]

dougpuob added a comment.

Replied comments by @aaron.ballman



================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:216
+        {"long",            "l"},
+        {"long long",       "ll"},
+        {"unsigned long",   "ul"},
----------------
aaron.ballman wrote:
> `unsigned long long` -> `ull`
OK.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:225
+        {"WORD",            "w"},
+        {"DWORD",           "dw"}};
+  // clang-format on
----------------
aaron.ballman wrote:
> `ULONG` -> `ul`
> `HANDLE` -> `h`
> 
OK.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:236
+      PrefixStr = "fn"; // Function Pointer
+    } else if (QT->isPointerType()) {
+      // clang-format off
----------------
aaron.ballman wrote:
> 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.
OK (size_t nLength --> cbLength)

About the `void something(const char *buffer, size_t length)` pattern. `char` is a special type which can express signed char and unsigned char.  One can express C string(ASCII), another expresses general data buffer(in hex). I think you are mentioning when it is a general data buffer. I agree with you if it changed the name from `buffer` to `szBuffer` for general data buffer is strange. I prefer to use `uint8_t` or `unsigned char` instead.

How about adding a new config option for it? like, `  - { key: readability-identifier-naming.HungarainNotation.DontChangeCharPointer   , value: 1 }` Users can make decision for their projects. For consistency with HN, the default will still be changed to `szBuffer` in your case.

If I add the option, does it make sense to you from your side?


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:309
+
+  if (PtrCount > 0) {
+    for (size_t Idx = 0; Idx < PtrCount; Idx++) {
----------------
aaron.ballman wrote:
> No need for this `if` statement, the `for` loop won't run anyway if `PtrCount == 0`.
OK! redundant code.


================
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);
----------------
aaron.ballman wrote:
> `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?
OK, I should do it. (ND instead of Decl.)

Use `Decl->getType()->getAsString()` is a good way. But HN is a strongly-typed notation, if users haven't specific the include directories, the checking result may look messy (it will be changed to unexpected type). So I parse a string with `SourceLocation`, just for user-friendly.

I understand your consideration, maintaining-friendly is also important. I can implement it with `Decl->getType()->getAsString()`, if my explanation can't convince you still. It is also fine to me. Or you think it just need a better appropriate function name?


================
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) {
----------------
aaron.ballman wrote:
> `const auto *` since the type is spelled out in the initialization.
OK.


================
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 =
----------------
aaron.ballman wrote:
> `const auto *` because the type is spelled out in the initialization.
OK.


================
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;
----------------
aaron.ballman wrote:
> Can you go with `ND` (or something else) instead of `Decl` since that's a type name?
It's my mistake, you mentioned last time. I will check them all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86671/new/

https://reviews.llvm.org/D86671