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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
