aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:185
+getHungarianNotationTypePrefix(const std::string &TypeName,
+ const NamedDecl *Decl) {
+ if (0 == TypeName.length()) {
----------------
Please don't use `Decl` as the identifier since it mirrors the name of a type.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:186
+ const NamedDecl *Decl) {
+ if (0 == TypeName.length()) {
+ return TypeName;
----------------
`if (TypeName.empty())`
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:208
+ {"wchar_t", "wc"},
+ {"short", "s"},
+ {"signed", "i"},
----------------
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
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:211
+ {"unsigned", "u"},
+ {"long", "l"}};
+ // clang-format on
----------------
How about: `long long`, `long double`, `ptrdiff_t`, and `void` (for `void *`
parameters)?
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:222
+ // clang-format off
+ const static llvm::StringMap<StringRef> NullString = {
+ {"char*", "sz"},
----------------
Are there special prefixes for `char8_t *`, `char16_t *`, or `char32_t *`?
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:288
+ }
+
+ return PrefixStr;
----------------
How about function pointers?
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:310
+ // Qualifier
+ "const", "volatile",
+ // Storage class specifiers
----------------
`restrict`?
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:314
+ // Constexpr specifiers
+ "constexpr", "constinit", "const_cast", "consteval"};
+
----------------
`const_cast` is not a constexpr specifier?
================
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) {
----------------
`pDecl` doesn't match our usual conventions. ;-)
================
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 =
----------------
Same here.
================
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);
----------------
`pNamedDecl` can be null, which could crash.
================
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) {
----------------
Missing a full stop at the end of the comment.
================
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(),
----------------
FWIW, we don't typically use top-level `const` on value types.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:563
+ const IdentifierNamingCheck::NamingStyle &Style,
+ const Decl *Decl) {
const std::string Fixed = fixupWithCase(
----------------
Similar naming issue here with `Decl` (elsewhere as well, I'll stop bringing
this one up).
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-identifier-naming.rst:21
+ - ``aNy_CasE``,
+ - ``szHungarianNotation``.
----------------
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.)
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-hungarian-notation.cpp:1
+#include <stddef.h>
+#include <stdint.h>
----------------
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.
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
