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

Douglas Chen via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Apr 9 02:20:33 PDT 2023


dougpuob marked 2 inline comments as done.
dougpuob added a comment.

In D147779#4251081 <https://reviews.llvm.org/D147779#4251081>, @PiotrZSL wrote:

> Missing release notes.

Thank you for reminding me.



================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:367-379
+        if (const auto *TD = dyn_cast<ValueDecl>(ND)) {
+          QualType QT = TD->getType();
+
+          size_t PtrCount = 0;
+          if (QT->isPointerType()) {
+            PtrCount = getAsteriskCount(Type);
+          }
----------------
PiotrZSL wrote:
> this function is already big, extract this to have something like:
> ```
> if (removeReudnantAsterisks(Type, ND))
> {
>     RedundantRemoved = true;
>     break;
> }
> ```
Good idea, thank you.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:696-702
+    const std::string &TypeName) const {
+  size_t Pos = TypeName.find('*');
+  size_t Count = 0;
+  for (; Pos < TypeName.length(); Pos++, Count++) {
+    if ('*' != TypeName[Pos])
+      break;
+  }
----------------
PiotrZSL wrote:
> what if type will be like std::optional<unsigned char**>, will this function also be executed ?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147779/new/

https://reviews.llvm.org/D147779



More information about the cfe-commits mailing list