[PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation
Douglas Chen via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 2 20:24:36 PDT 2020
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
More information about the cfe-commits
mailing list