aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:482
Styles.emplace_back(IdentifierNamingCheck::NamingStyle{
- std::move(CaseOptional), std::move(Prefix), std::move(Postfix)});
- else
+ std::move(CaseOptional), std::move(Prefix), std::move(Postfix),
+ std::move(HPOpt), HNOption});
----------------
Drive-by note that's not something you need to address: these calls to
`std::move()` for prefix and postfix are rather suspect given that the
parameters in the constructor are both `const std::string&`.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:483
+ std::move(CaseOptional), std::move(Prefix), std::move(Postfix),
+ std::move(HPOpt), HNOption});
+ } else {
----------------
Would it make sense to declare `HNOption` as a typical local variable (no need
for the `clearAll()` API since you can default construct properly), and pass an
rvalue reference rather than an lvalue reference (so call std::move()) here, as
with the other arguments)?
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:549
+ for (const auto &CStr : HNOption.CString) {
+ auto Key = CStr.getKey().str();
+ if (ModifiedTypeName.find(Key) == 0) {
----------------
You shouldn't use `auto` when the type isn't spelled out in the initialization.
(Same below)
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:631
+
+ if (CRD->isUnion()) {
return "";
----------------
Elide braces around single-line if statements, per our usual coding guidelines.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:635-636
- if (clang::Decl::Kind::EnumConstant == Decl->getKind() ||
- clang::Decl::Kind::CXXMethod == Decl->getKind()||
- clang::Decl::Kind::Function == Decl->getKind()) {
+ if (CRD->isStruct()) {
+ if (!isHungarianNotationOptionEnabled("TreatStructAsClass",
+ HNOption.General)) {
----------------
Combine into a single `if`?
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:644
+ bool IsAbstractClass = false;
+ for (const auto *Method : CRD->methods()) {
+ if (Method->isPure()) {
----------------
No need to do this manually, you can use `CXXRecordData::isAbstract()` since
it's already computed.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:651-658
+ std::string Prefix;
+ if (IsAbstractClass) {
+ Prefix = "I";
+ } else {
+ Prefix = "C";
+ }
+
----------------
`return CRD->isAbstract() ? "I" : "C";`
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:662-666
+ std::string Name = ECD->getType().getAsString();
+ if (std::string::npos != Name.find("enum")) {
+ Name = Name.substr(strlen("enum"), Name.length() - strlen("enum"));
+ Name = Name.erase(0, Name.find_first_not_of(" "));
+ }
----------------
This is a bit worrying -- can you not look at the `DeclContext` for the
`EnumConstantDecl` to find the parent `EnumDecl` and ask for its name directly?
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:703
+
+static std::string getDeclTypeName(const clang::NamedDecl *ND) {
+ const auto VD = dyn_cast<ValueDecl>(ND);
----------------
You can drop the `clang::` bit.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:704
+static std::string getDeclTypeName(const clang::NamedDecl *ND) {
+ const auto VD = dyn_cast<ValueDecl>(ND);
+ if (!VD) {
----------------
When using type deduction, the coding style is to put all pointers and
qualifiers on the declaration as well (so we don't have to infer them from
context), so this should be `const auto *`.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:806
+static std::string getHungarianNotationPrefix(
+ const clang::Decl *D,
+ IdentifierNamingCheck::HungarianNotationOption &HNOption) {
----------------
Can drop the `clang::` bit.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:808-809
+ IdentifierNamingCheck::HungarianNotationOption &HNOption) {
+ const auto ND = dyn_cast<NamedDecl>(D);
+ if (!ND) {
+ return "";
----------------
`const auto *` and elide braces (I'll stop pointing these out, can you do a
pass over the whole check?)
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
