[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:185
+getHungarianNotationTypePrefix(const std::string &TypeName,
+                               const NamedDecl *Decl) {
+  if (0 == TypeName.length()) {
----------------
Please don't use `Decl` as the identifier since it mirrors the name of a type.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:186
+                               const NamedDecl *Decl) {
+  if (0 == TypeName.length()) {
+    return TypeName;
----------------
`if (TypeName.empty())`


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:208
+        {"wchar_t",         "wc"},
+        {"short",           "s"},
+        {"signed",          "i"},
----------------
It's been a long while since I thought about Hungarian notation, but I thought this was prefixed with `w` because it's word-sized (and `dw` for double word size)?

FWIW: https://docs.microsoft.com/en-us/windows/win32/stg/coding-style-conventions


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:211
+        {"unsigned",        "u"},
+        {"long",            "l"}};
+  // clang-format on
----------------
How about: `long long`, `long double`, `ptrdiff_t`, and `void` (for `void *` parameters)?


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:222
+      // clang-format off
+      const static llvm::StringMap<StringRef> NullString = {
+        {"char*",     "sz"},
----------------
Are there special prefixes for `char8_t *`, `char16_t *`, or `char32_t *`?


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:288
+  }
+
+  return PrefixStr;
----------------
How about function pointers?


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:310
+        // Qualifier
+        "const", "volatile",
+        // Storage class specifiers
----------------
`restrict`?


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:314
+        // Constexpr specifiers
+        "constexpr", "constinit", "const_cast", "consteval"};
+
----------------
`const_cast` is not a constexpr specifier?


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:384
+static std::string fixupWithCase(const StringRef &Type, const StringRef &Name,
+                                 const Decl *pDecl,
                                  IdentifierNamingCheck::CaseType Case) {
----------------
`pDecl` doesn't match our usual conventions. ;-)


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:478
+  case IdentifierNamingCheck::CT_HungarianNotation: {
+    const NamedDecl *pNamedDecl = dyn_cast<NamedDecl>(pDecl);
+    const std::string TypePrefix =
----------------
Same here.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:479
+    const NamedDecl *pNamedDecl = dyn_cast<NamedDecl>(pDecl);
+    const std::string TypePrefix =
+        getHungarianNotationTypePrefix(Type.str(), pNamedDecl);
----------------
`pNamedDecl` can be null, which could crash.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:483
+    for (size_t Idx = 0; Idx < Words.size(); Idx++) {
+      // Skip first part if it's a lowercase string
+      if (Idx == 0) {
----------------
Missing a full stop at the end of the comment.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:485
+      if (Idx == 0) {
+        const bool LowerAlnum =
+            std::all_of(Words[Idx].begin(), Words[Idx].end(),
----------------
FWIW, we don't typically use top-level `const` on value types.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:563
+               const IdentifierNamingCheck::NamingStyle &Style,
+               const Decl *Decl) {
   const std::string Fixed = fixupWithCase(
----------------
Similar naming issue here with `Decl` (elsewhere as well, I'll stop bringing this one up).


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:21
+ - ``aNy_CasE``,
+ - ``szHungarianNotation``.
 
----------------
We should probably document what prefixes we use for Hungarian notation, since there may be some alternatives in the wild. (It's not clear to me whether we should make the prefixes configurable or not.)


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp:1
+#include <stddef.h>
+#include <stdint.h>
----------------
Test cases should not include system headers (that makes the test sensitive to the system which is a problem). You should lift the declarations you need out of the header file and manually declare them in the test file.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671