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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
