[PATCH] D72217: [clang-tidy] Added readability-qualified-auto check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:31
+  Const,
+  Volatile // Ignore restrict as not c++ keyword.
+};
----------------
We still support `restrict` in C++ though: https://godbolt.org/z/CT5nw2

Also, one thing that comes up whenever we talk about qualifiers are type attributes like address spaces. I have no idea how those interact with type deduction (simple tests show that it does seem to be deduced: https://godbolt.org/z/LtFAMJ), but I would imagine we would want (at least an option) to be able to ensure those get written out explicitly as well. (If this turns out to be difficult, it could be done in a follow-up patch.)


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:93
+bool isPointerConst(QualType QType) {
+  auto Pointee = QType.getTypePtr()->getPointeeType();
+  if (Pointee.isNull())
----------------
You shouldn't use `auto` here as the type is not spelled out in the initialization.


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:94-95
+  auto Pointee = QType.getTypePtr()->getPointeeType();
+  if (Pointee.isNull())
+    return Pointee.isLocalConstQualified();
+  return Pointee.isConstQualified();
----------------
Can you explain why this code is needed? If the pointee is null, there's been some sort of error, I believe.

I am not certain this function is needed, I think `QType->getPointeeType().isConstQualified()` likely suffices.


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:101-102
+  auto Pointee =
+      dyn_cast<AutoType>(QType.getTypePtr()->getPointeeType().getTypePtr())
+          ->getDeducedType();
+  if (Pointee.isNull())
----------------
You can skip calling `getTypePtr()` and just use the `->` overload.


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:103-104
+          ->getDeducedType();
+  if (Pointee.isNull())
+    return Pointee.isLocalConstQualified();
+  return Pointee.isConstQualified();
----------------
Same observations here as above.


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:159
+void QualifiedAutoCheck::check(const MatchFinder::MatchResult &Result) {
+  if (auto Var = Result.Nodes.getNodeAs<VarDecl>("auto")) {
+    bool IsPtrConst = isPointerConst(Var->getType());
----------------
`auto *`


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:160
+  if (auto Var = Result.Nodes.getNodeAs<VarDecl>("auto")) {
+    bool IsPtrConst = isPointerConst(Var->getType());
+    SourceRange TypeSpecifier;
----------------
Might as well move this closer to where it's actually used.


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

https://reviews.llvm.org/D72217