dougpuob added a comment. Please no worry to give me your suggestions and feedback.
================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:185 +getHungarianNotationTypePrefix(const std::string &TypeName, + const NamedDecl *Decl) { + if (0 == TypeName.length()) { ---------------- aaron.ballman wrote: > Please don't use `Decl` as the identifier since it mirrors the name of a type. OK~ I will fix it. Thank you. ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:186 + const NamedDecl *Decl) { + if (0 == TypeName.length()) { + return TypeName; ---------------- aaron.ballman wrote: > `if (TypeName.empty())` OK~ I will fix it. Thank you. ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:208 + {"wchar_t", "wc"}, + {"short", "s"}, + {"signed", "i"}, ---------------- 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; ``` ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:211 + {"unsigned", "u"}, + {"long", "l"}}; + // clang-format on ---------------- aaron.ballman wrote: > How about: `long long`, `long double`, `ptrdiff_t`, and `void` (for `void *` > parameters)? OK~ I will add them. Thank you. The `void*` and `ptrdiff_t` starts with `p`. ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:222 + // clang-format off + const static llvm::StringMap<StringRef> NullString = { + {"char*", "sz"}, ---------------- aaron.ballman wrote: > Are there special prefixes for `char8_t *`, `char16_t *`, or `char32_t *`? I will add them too. Thank you. ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:288 + } + + return PrefixStr; ---------------- aaron.ballman wrote: > How about function pointers? Nice consideration, thank you. I will add the feature. function pointers will start with `fn`. I will add a test like this, ``` typedef void (*FUNC_PTR_HELLO)(); FUNC_PTR_HELLO Hello = NULL; // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for variable 'Hello' [readability-identifier-naming] // CHECK-FIXES: {{^}}FUNC_PTR_HELLO fnHello = NULL; ``` ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:310 + // Qualifier + "const", "volatile", + // Storage class specifiers ---------------- aaron.ballman wrote: > `restrict`? I will check it again. Thank you. ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:314 + // Constexpr specifiers + "constexpr", "constinit", "const_cast", "consteval"}; + ---------------- aaron.ballman wrote: > `const_cast` is not a constexpr specifier? I will check it again. Thank you. ================ 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) { ---------------- aaron.ballman wrote: > `pDecl` doesn't match our usual conventions. ;-) Oops! I will fix it later. ================ 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 = ---------------- aaron.ballman wrote: > Same here. I will fix too. ================ 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); ---------------- 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? ================ 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) { ---------------- aaron.ballman wrote: > Missing a full stop at the end of the comment. OK, I will fix it later. Thank you. ================ 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(), ---------------- 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? ================ Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:563 + const IdentifierNamingCheck::NamingStyle &Style, + const Decl *Decl) { const std::string Fixed = fixupWithCase( ---------------- aaron.ballman wrote: > Similar naming issue here with `Decl` (elsewhere as well, I'll stop bringing > this one up). OK, I will fix it later. ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:21 + - ``aNy_CasE``, + - ``szHungarianNotation``. ---------------- 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? ================ Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp:1 +#include <stddef.h> +#include <stdint.h> ---------------- aaron.ballman wrote: > 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. OK, make sense. I will fix it later. Thank you. 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