dougpuob added a comment.
Reply code review suggestions. I will upload my change later.
================
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 {
----------------
aaron.ballman wrote:
> 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)?
OK~ I moved the `HNOption` to IdentifierNamingCheck class as its private data
member. Then give value by constructor initializer list (instead of
`clearAll()`).
================
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) {
----------------
aaron.ballman wrote:
> You shouldn't use `auto` when the type isn't spelled out in the
> initialization. (Same below)
OK~ Thank you. I will check them.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:631
+
+ if (CRD->isUnion()) {
return "";
----------------
aaron.ballman wrote:
> Elide braces around single-line if statements, per our usual coding
> guidelines.
OK.
================
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)) {
----------------
aaron.ballman wrote:
> Combine into a single `if`?
OK.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:644
+ bool IsAbstractClass = false;
+ for (const auto *Method : CRD->methods()) {
+ if (Method->isPure()) {
----------------
aaron.ballman wrote:
> No need to do this manually, you can use `CXXRecordData::isAbstract()` since
> it's already computed.
Nice, thank you.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:651-658
+ std::string Prefix;
+ if (IsAbstractClass) {
+ Prefix = "I";
+ } else {
+ Prefix = "C";
+ }
+
----------------
aaron.ballman wrote:
> `return CRD->isAbstract() ? "I" : "C";`
OK.
================
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(" "));
+ }
----------------
aaron.ballman wrote:
> 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?
YES. I get the its `QualType` from `EnumConstantDecl::getType()`. Then get
`EnumDecl` name via `QualType::getAsString()` (if enum tag name is "DataType",
the string I get is "enum DataType").
```
static std::string getHungarianNotationEnumPrefix(const EnumConstantDecl *ECD) {
std::string Name = ECD->getType().getAsString();
// ...
}
```
================
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);
----------------
aaron.ballman wrote:
> You can drop the `clang::` bit.
OK.
================
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) {
----------------
aaron.ballman wrote:
> 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 *`.
OK, thank you.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:806
+static std::string getHungarianNotationPrefix(
+ const clang::Decl *D,
+ IdentifierNamingCheck::HungarianNotationOption &HNOption) {
----------------
aaron.ballman wrote:
> Can drop the `clang::` bit.
OK.
================
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 "";
----------------
aaron.ballman wrote:
> `const auto *` and elide braces (I'll stop pointing these out, can you do a
> pass over the whole check?)
Sure! it's what should I do, you have helped me a lots.
Repository:
rG LLVM Github Monorepo
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
