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

[Aaron Ballman via Phabricator via cfe-commits]

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