[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:208
+        {"wchar_t",         "wc"},
+        {"short",           "s"},
+        {"signed",          "i"},
----------------
dougpuob wrote:
> aaron.ballman wrote:
> > 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
> It is a good question.
> 
> Microsoft redefines them, so I would like to add them as part of mapping table, `HungarianNotationTable`. That means the map include primitive types and partial windows data types. 
> 
> ```
> // Windows Data Type
> {"BOOL",            "b"},   // typedef int BOOL;
> {"BOOLEAN",         "b"},   // typedef BYTE BOOLEAN;
> {"BYTE",            "by"},  // typedef unsigned char BYTE;
> {"WORD",            "w"},   // typedef unsigned short WORD;
> {"DWORD",           "dw"},  // typedef unsigned long DWORD;
> ```
> 
> The result will like the following,
> ```
> WORD wVid = 0x8086;
> DWORD dwVidDid = 0x80861234;
> ```
I think this would be user-friendly behavior, though I'm curious if we'll run into any conflicting notations this way.


================
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);
----------------
dougpuob wrote:
> aaron.ballman wrote:
> > `pNamedDecl` can be null, which could crash.
> Make sense. If it was you, will you check it at the caller or in the callee? and could I know the reason?
I'd handle it in `getHungarianNotationTypePrefix()` along with the check for an empty type name as both seem like they're checking for a degenerate case.


================
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(),
----------------
dougpuob wrote:
> aaron.ballman wrote:
> > FWIW, we don't typically use top-level `const` on value types.
> OK. But I am curious about it. Is it a rule in this project? or it is a flaw?
Not really a flaw (there's nothing wrong with top-level `const` on value types), but sort of a rule. Our coding style basically says to try to match the local styles used by the project. We've never really done top-level `const` anywhere else in the project, so any time someone introduces it, I usually ask for it to be removed. Also, as much as I love `const` correctness, many of our APIs are not super `const` correct (we're getting much better though), so it can also make code somewhat awkward when you hit an older API that accepts something non-`const`.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:21
+ - ``aNy_CasE``,
+ - ``szHungarianNotation``.
 
----------------
dougpuob wrote:
> aaron.ballman wrote:
> > 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.)
> Agree with you consideration.
> 
> 1. I will add more detail about the table, `HungarianNotationTable` .
> 
> 2. I would like treat it as a default table for this moment. Because I don't know does it make sense that make user can customized it in `.clang-tidy` file. The priority in config is higher than default table.  Show my idea as the following,
> 
> ```
> Checks: '-*,readability-identifier-naming'
> CheckOptions:
>   - { key: readability-identifier-naming.VariableCase                   , value: szHungarianNotion }
>   - { key: readability-identifier-naming.HungarianWordList.uint8        , value: u8                }
>   - { key: readability-identifier-naming.HungarianWordList.uint16       , value: u16               }
>   - { key: readability-identifier-naming.HungarianWordList.uint32       , value: u32               }
>   - { key: readability-identifier-naming.HungarianWordList.uint64       , value: u64               }
>   - { key: readability-identifier-naming.HungarianWordList.unsigned_long, value: ul                }
>   - { key: readability-identifier-naming.HungarianWordList.long_long    , value: ll                }
> ```
> 
> How about it?
I think allowing the prefixes to be configurable is a good idea and I agree that we should have a sensible default table (what you've proposed so far seems like a reasonable default to me). One thing we should be sure we support (and have tests for) is the case where the user wants to use the default table but change only one prefix from it. e.g., they like the defaults but change `long_long` from `ll` to `super_`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D86671