dougpuob added a comment.
Replied comments by @aaron.ballman
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:216
+ {"long", "l"},
+ {"long long", "ll"},
+ {"unsigned long", "ul"},
----------------
aaron.ballman wrote:
> `unsigned long long` -> `ull`
OK.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:225
+ {"WORD", "w"},
+ {"DWORD", "dw"}};
+ // clang-format on
----------------
aaron.ballman wrote:
> `ULONG` -> `ul`
> `HANDLE` -> `h`
>
OK.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:236
+ PrefixStr = "fn"; // Function Pointer
+ } else if (QT->isPointerType()) {
+ // clang-format off
----------------
aaron.ballman wrote:
> I'm not certain how valid it is to look at just the type and decide that it's
> a null-terminated string. For instance, the following is not an uncommon
> pattern: `void something(const char *buffer, size_t length);` and it would be
> a bit strange to change that into `szBuffer` as that would give an indication
> that the buffer is null terminated. You could look at surrounding code for
> additional information though, like other parameters in a function
> declaration. As an aside, this sort of heuristic search may also allow you to
> change `length` into `cbLength` instead of `nLength` for conventions like the
> Microsoft one.
>
> However, for local variables, I don't see how you could even come up with a
> heuristic like you could with parameters, so I'm not certain what the right
> answer is here.
OK (size_t nLength --> cbLength)
About the `void something(const char *buffer, size_t length)` pattern. `char`
is a special type which can express signed char and unsigned char. One can
express C string(ASCII), another expresses general data buffer(in hex). I think
you are mentioning when it is a general data buffer. I agree with you if it
changed the name from `buffer` to `szBuffer` for general data buffer is
strange. I prefer to use `uint8_t` or `unsigned char` instead.
How about adding a new config option for it? like, ` - { key:
readability-identifier-naming.HungarainNotation.DontChangeCharPointer ,
value: 1 }` Users can make decision for their projects. For consistency with
HN, the default will still be changed to `szBuffer` in your case.
If I add the option, does it make sense to you from your side?
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:309
+
+ if (PtrCount > 0) {
+ for (size_t Idx = 0; Idx < PtrCount; Idx++) {
----------------
aaron.ballman wrote:
> No need for this `if` statement, the `for` loop won't run anyway if `PtrCount
> == 0`.
OK! redundant code.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:319
+std::string
+IdentifierNamingCheck::getDeclTypeName(const clang::NamedDecl *Decl) const {
+ const ValueDecl *ValDecl = dyn_cast<ValueDecl>(Decl);
----------------
aaron.ballman wrote:
> `ND` instead of `Decl`.
>
> The function name doesn't really help me to understand why you'd call this as
> opposed to getting the type information as a string from the `NamedDecl`
> itself. I'm a bit worried about maintaining this code as the language evolves
> -- Clang will get new keywords, and someone will have to remember to come
> update this code. Could you get away with using
> `Decl->getType()->getAsString()` and working with that rather than going back
> to the original source code and trying to parse manually?
OK, I should do it. (ND instead of Decl.)
Use `Decl->getType()->getAsString()` is a good way. But HN is a strongly-typed
notation, if users haven't specific the include directories, the checking
result may look messy (it will be changed to unexpected type). So I parse a
string with `SourceLocation`, just for user-friendly.
I understand your consideration, maintaining-friendly is also important. I can
implement it with `Decl->getType()->getAsString()`, if my explanation can't
convince you still. It is also fine to me. Or you think it just need a better
appropriate function name?
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:320
+IdentifierNamingCheck::getDeclTypeName(const clang::NamedDecl *Decl) const {
+ const ValueDecl *ValDecl = dyn_cast<ValueDecl>(Decl);
+ if (!ValDecl) {
----------------
aaron.ballman wrote:
> `const auto *` since the type is spelled out in the initialization.
OK.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:554
+ case IdentifierNamingCheck::CT_HungarianNotation: {
+ const NamedDecl *ND = dyn_cast<NamedDecl>(InputDecl);
+ const std::string TypePrefix =
----------------
aaron.ballman wrote:
> `const auto *` because the type is spelled out in the initialization.
OK.
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:39
+ std::string getDeclTypeName(const clang::NamedDecl *Decl) const;
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
----------------
aaron.ballman wrote:
> Can you go with `ND` (or something else) instead of `Decl` since that's a
> type name?
It's my mistake, you mentioned last time. I will check them all.
Repository:
rG LLVM Github Monorepo
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
