[PATCH] D86671: [clang-tidy] Add new case type to check variables with Hungarian notation

[Douglas Chen via Phabricator via cfe-commits]

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