[clang-tidy] Fix hungarian notation failed to indicate the number of asterisks in check-clang-extra-clang-tidy-checkers-readability

[dougpuob]

Summary

Fix hungarian notation failed to indicate the number of asterisks for the pointers of multiple word types.

• WRONG: unsigned char* value : value --> ucValue
• RIGHT: unsigned cahr* value : value --> pucValue
• RIGHT: unsigned char** value : value --> ppucValue

[PiotrZSL]

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

376–377

this function is already big, extract this to have something like:

if (removeReudnantAsterisks(Type, ND))
{
    RedundantRemoved = true;
    break;
}

695–701

what if type will be like std::optional<unsigned char**>, will this function also be executed ?

[PiotrZSL]

Missing release notes.

[dougpuob]

In D147779#4251081, @PiotrZSL wrote:

Missing release notes.

Thank you for reminding me.

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

376–377

Good idea, thank you.

695–701

Yes, this function is also executed in this case, but the std::optional<unsigned char**> is not in the supported list so the case will be ignored. Even though it is still weird, I will add some code to remove template parameters in the getDeclTypeName() function, making it clean.

• OLD: std::optional<unsigned char**> --> std::optional<unsigned char (weird result)
• NEW: std::optional<unsigned char**> --> std::optional (removed template parameters)

Thank you for reminding me.

[dougpuob]

• Extract method
• Remove template parameters in the getDeclTypeName() function

[PiotrZSL]

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

706

const std::string&

clang-tools-extra/docs/ReleaseNotes.rst

324–326

this list should by sorted by a full check name, also there are already fixes for readability-identifier-naming, check if you could merge this to them.

[PiotrZSL]

LGTM, except pointed out issues with order of fixes in release note and missing const.

[dougpuob]

• Improved suggestions in code review

[amurzeau]

Thanks ! LGTM, nothing to add.

[dougpuob]

Hi @PiotrZSL
I do not have permission to land the change. Can you please help me to land this change?

Another related question is that can I know how you land diffs? Take this diff as an example. Is it correct with arc land --onto main --revision D147779?

[PiotrZSL]

arc land should work, I usually use arc patch --nobranch to check things before committing and then git push.

[dougpuob]

In D147779#4257212, @PiotrZSL wrote:

arc land should work, I usually use arc patch --nobranch to check things before committing and then git push.

Hi @PiotrZSL, thank you for the help :)

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h

struct NamingStyle {
     HungarianPrefixType HPType;
   };
 
   struct HungarianNotation {
   public:
     bool checkOptionValid(int StyleKindIndex) const;
     bool isOptionEnabled(StringRef OptionKey,
                          const llvm::StringMap<std::string> &StrMap) const;
+
+    size_t getAsteriskCount(const std::string &TypeName) const;
+    size_t getAsteriskCount(const std::string &TypeName,
+                            const NamedDecl *ND) const;
+
     void loadDefaultConfig(
         IdentifierNamingCheck::HungarianNotationOption &HNOption) const;
     void loadFileConfig(
         const ClangTidyCheck::OptionsView &Options,
         IdentifierNamingCheck::HungarianNotationOption &HNOption) const;
 
     bool removeDuplicatedPrefix(
         SmallVector<StringRef, 8> &Words,

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp

if (StrLen > 0) {
     for (StringRef Kw : Keywords) {
       for (size_t Pos = 0;
            (Pos = Type.find(Kw.data(), Pos)) != std::string::npos;) {
         Type.replace(Pos, Kw.size(), "");
       }
     }
     TypeName = Type.erase(0, Type.find_first_not_of(" "));
 
+    // Remove template parameters
+    const size_t Pos = Type.find("<");
+    if (Pos != std::string::npos) {
+      TypeName = Type.erase(Pos, Type.size() - Pos);
+    }
+
     // Replace spaces with single space.
     for (size_t Pos = 0; (Pos = Type.find("  ", Pos)) != std::string::npos;
          Pos += strlen(" ")) {
       Type.replace(Pos, strlen("  "), " ");
     }
 
     // Replace " &" with "&".
     for (size_t Pos = 0; (Pos = Type.find(" &", Pos)) != std::string::npos;
if (StrLen > 0) {
 
     // Remove redundant tailing.
     static constexpr StringRef TailsOfMultiWordType[] = {
         " int", " char", " double", " long", " short"};
     bool RedundantRemoved = false;
     for (auto Kw : TailsOfMultiWordType) {
       size_t Pos = Type.rfind(Kw.data());
       if (Pos != std::string::npos) {
-        Type = Type.substr(0, Pos + Kw.size());
+        const size_t PtrCount = getAsteriskCount(Type, ND);
+        Type = Type.substr(0, Pos + Kw.size() + PtrCount);
         RedundantRemoved = true;
         break;
       }
     }
+
     TypeName = Type.erase(0, Type.find_first_not_of(" "));
     if (!RedundantRemoved) {
       std::size_t FoundSpace = Type.find(" ");
       if (FoundSpace != std::string::npos)
         Type = Type.substr(0, FoundSpace);
     }
 
     TypeName = Type.erase(0, Type.find_first_not_of(" "));
if (const auto *TD = dyn_cast<ValueDecl>(ND)) {
     } else if (QT->isReferenceType()) {
       size_t Pos = ModifiedTypeName.find_last_of("&");
       if (Pos != std::string::npos)
         ModifiedTypeName = ModifiedTypeName.substr(0, Pos);
     }
   }
 
   // Pointers
-  size_t PtrCount = [&](std::string TypeName) -> size_t {
-    size_t Pos = TypeName.find('*');
-    size_t Count = 0;
-    for (; Pos < TypeName.length(); Pos++, Count++) {
-      if ('*' != TypeName[Pos])
-        break;
-    }
-    return Count;
-  }(ModifiedTypeName);
+  size_t PtrCount = getAsteriskCount(ModifiedTypeName);
   if (PtrCount > 0) {
     ModifiedTypeName = [&](std::string Str, StringRef From, StringRef To) {
       size_t StartPos = 0;
       while ((StartPos = Str.find(From.data(), StartPos)) !=
              std::string::npos) {
         Str.replace(StartPos, From.size(), To.data());
         StartPos += To.size();
       }
std::string IdentifierNamingCheck::HungarianNotation::getEnumPrefix(
 
   std::string Initial;
   for (StringRef Word : Words)
     Initial += tolower(Word[0]);
 
   return Initial;
 }
 
+size_t IdentifierNamingCheck::HungarianNotation::getAsteriskCount(
+    const std::string &TypeName) const {
+  size_t Pos = TypeName.find('*');
+  size_t Count = 0;
+  for (; Pos < TypeName.length(); Pos++, Count++) {
+    if ('*' != TypeName[Pos])
+      break;
+  }
+  return Count;
+}
+
+size_t IdentifierNamingCheck::HungarianNotation::getAsteriskCount(
+    const std::string &TypeName, const NamedDecl *ND) const {
+  size_t PtrCount = 0;
+  if (const auto *TD = dyn_cast<ValueDecl>(ND)) {
+    QualType QT = TD->getType();
+    if (QT->isPointerType())
+      PtrCount = getAsteriskCount(TypeName);
+  }
+  return PtrCount;
+}
+
 void IdentifierNamingCheck::HungarianNotation::loadDefaultConfig(
     IdentifierNamingCheck::HungarianNotationOption &HNOption) const {
 
   // Options
   static constexpr std::pair<StringRef, StringRef> General[] = {
       {"TreatStructAsClass", "false"}};
   for (const auto &G : General)
     HNOption.General.try_emplace(G.first, G.second);

clang-tools-extra/docs/ReleaseNotes.rst

- Renamed `HungarianNotation.CString` options `CharPrinter` and
   <clang-tidy/checks/readability/identifier-naming>` check.
 
 - Updated the Hungarian prefixes for enums in C files to match those used in C++
   files for improved readability, as checked by :doc:`readability-identifier-naming
   <clang-tidy/checks/readability/identifier-naming>`. To preserve the previous
   behavior of using `i` as the prefix for enum tags, set the `EnumConstantPrefix`
   option to `i` instead of using `EnumConstantHungarianPrefix`.
 
+- Fixed a hungarian notation issue in :doc:`readability-identifier-naming
+  <clang-tidy/checks/readability/identifier-naming>` which failed to indicate
+  the number of asterisks.
+
 - Fixed a false positive in :doc:`readability-implicit-bool-conversion
   <clang-tidy/checks/readability/implicit-bool-conversion>` check warning would
   be unnecessarily emitted for explicit cast using direct list initialization.
 
 - Added support to optionally ignore user-defined literals in
   :doc:`readability-magic-numbers<clang-tidy/checks/readability/magic-numbers>`.
 
 - Fixed a false positive in :doc:`readability-container-size-empty
- Improved :doc:`modernize-concat-nested-namespaces
   <clang-tidy/checks/modernize/concat-nested-namespaces>` to fix incorrect fixes when
   using macro between namespace declarations and false positive when using namespace
   with attributes.
 
 - Fixed a false positive in :doc:`performance-no-automatic-move
   <clang-tidy/checks/performance/no-automatic-move>` when warning would be
   emitted for a const local variable to which NRVO is applied.
 
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to clang-include-fixer
 -----------------------------------
 

clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-hungarian-notation-c-language.c

 int8_t *ValueI8Ptr;
 // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for global pointer 'ValueI8Ptr' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}int8_t *pi8ValueI8Ptr;
 
 uint8_t *ValueU8Ptr;
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for global pointer 'ValueU8Ptr' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}uint8_t *pu8ValueU8Ptr;
 
+unsigned char *ValueUcPtr;
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for global pointer 'ValueUcPtr' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}unsigned char *pucValueUcPtr;
+
+unsigned char **ValueUcPtr2;
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: invalid case style for global pointer 'ValueUcPtr2' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}unsigned char **ppucValueUcPtr2;
+
 void MyFunc2(void* Val){}
 // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: invalid case style for pointer parameter 'Val' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}void MyFunc2(void* pVal){}
 
 
 //===----------------------------------------------------------------------===//
 // Various types
 //===----------------------------------------------------------------------===//

clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-hungarian-notation-cfgfile.cpp

 int8_t *ValueI8Ptr;
 // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for global pointer 'ValueI8Ptr' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}int8_t *custpcusti8ValueI8Ptr;
 
 uint8_t *ValueU8Ptr;
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for global pointer 'ValueU8Ptr' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}uint8_t *custpcustu8ValueU8Ptr;
 
+unsigned char *ValueUcPtr;
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for global pointer 'ValueUcPtr' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}unsigned char *custpcustucValueUcPtr;
+
+unsigned char **ValueUcPtr2;
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: invalid case style for global pointer 'ValueUcPtr2' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}unsigned char **custpcustpcustucValueUcPtr2;
+
 void MyFunc2(void* Val){}
 // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: invalid case style for pointer parameter 'Val' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}void MyFunc2(void* custpcustvVal){}
 
 
 //===----------------------------------------------------------------------===//
 // Reference
 //===----------------------------------------------------------------------===//

clang-tools-extra/test/clang-tidy/checkers/readability/identifier-naming-hungarian-notation.cpp

 int8_t *ValueI8Ptr;
 // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: invalid case style for global pointer 'ValueI8Ptr' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}int8_t *pi8ValueI8Ptr;
 
 uint8_t *ValueU8Ptr;
 // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: invalid case style for global pointer 'ValueU8Ptr' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}uint8_t *pu8ValueU8Ptr;
 
+unsigned char *ValueUcPtr;
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for global pointer 'ValueUcPtr' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}unsigned char *pucValueUcPtr;
+
+unsigned char **ValueUcPtr2;
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: invalid case style for global pointer 'ValueUcPtr2' [readability-identifier-naming]
+// CHECK-FIXES: {{^}}unsigned char **ppucValueUcPtr2;
+
 void MyFunc2(void* Val){}
 // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: invalid case style for pointer parameter 'Val' [readability-identifier-naming]
 // CHECK-FIXES: {{^}}void MyFunc2(void* pVal){}
 
 
 //===----------------------------------------------------------------------===//
 // Reference
 //===----------------------------------------------------------------------===//