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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

+1 for the check from me as well :)



================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:19
+  if (!getLangOpts().CPlusPlus) {
+    // auto deduction not used in c
+    return;
----------------
Please make the comments full sentences with punctuation.

This `if` has only one statement, so please ellide the braces - by coding standard.


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:48
+  return CharSourceRange::getTokenRange(Var->getBeginLoc(),
+                                        getTokLocationBeforeName(Var));
+}
----------------
thats dangerous. Your source location could be invalid or a macroID, I would rather make these `llvm::Optional`-


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:61
+      Diag << FixItHint::CreateReplacement(
+          FixItRange, IsPtrConst ? "const auto *const " : "auto *const ");
+    } else {
----------------
the outer const might be debatable. some code-bases don't want the `* const`, i think neither does LLVM. maybe that could be configurable?


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:71
+    if (!Var->getType().getTypePtr()->getPointeeType().isConstQualified()) {
+      // Pointer isn't const, no need to add const qualifier
+      return;
----------------
please make that comment a full sentence with punctuation and ellide the braces.


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:79
+            .isConstQualified();
+    if (!IsAutoPtrConst) {
+      return;
----------------
braces.


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:87
+
+    if (Var->getType().isConstQualified()) {
+      Diag << FixItHint::CreateReplacement(FixItRange, "const auto *const ");
----------------
braces.


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:96
+    if (!Var->getType().getTypePtr()->getPointeeType().isConstQualified()) {
+      // Reference isn't const, no need to add const qualifier
+      return;
----------------
full sentence.


================
Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:104
+            .isConstQualified();
+    if (!IsAutoRefConst) {
+      return;
----------------
braces.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst:45
+This check helps to enforce this `LLVM Coding Standards recommendation
+<https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto>`_.
----------------
I think this justifies an alias into the llvm-module we have.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-qualified-auto.cpp:53
+}
+
+namespace std {
----------------
please add some tests with typedefs and macro-interference in the definitions.

Typedefs i am interested in: (names are up to you, just to get the idea)
```
using MyPtr = int *;
using MyRef = int&;
using CMyPtr = const int *;
using CCMyPtr = const int * const;
using CMyRef = const int&;
```
I believe resolves the typedefs and becomes the reference/pointer, but that needs some test.

The transformation should not happen if the code comes from a macro ID
```
int* get_ptr() { return new int(42); }
#define AUTO auto
AUTO my_variable = get_pointer();
```
Transformation should not happen in this case, because the macro might be used in different places and macros are complicated.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-qualified-auto.cpp:100
+  loopPtr(PtrVec, CPtrVec);
+}
----------------
What about 
```
auto my_function -> int * { /* foo */ }
```
other contexts for `auto` need to be considered (lambdas, others?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72217