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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
