This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Added readability-qualified-auto check
ClosedPublic

Authored by njames93 on Jan 4 2020, 7:44 PM.

Details

Summary

Adds a check that detects any auto variables that are deduced to a pointer or a const pointer then adds in the const and asterisk according. Will also check auto L value references that could be written as const. This relates to the coding standard https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto

Diff Detail

Event Timeline

njames93 created this revision.Jan 4 2020, 7:44 PM

side note when creating this, the add_new_check.py file hasn't been updated in relation to this commit https://github.com/llvm/llvm-project/commit/d2c9c9157b0528436d3b9f5e22c872f0ee6509a2. This results in a malformed rst file. In this patch I have just manually set it up correctly, but a fix will need to be made for the add_new_check.py

May be check belong to LLVM module?

clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
70

Unnecessary empty line.

clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst
7

Please make first statement same as Release Notes.

14

Please separate with empty line.

26

Unnecessary empty line.

31

Please separate with empty line.

44

Unnecessary empty line.

Eugene.Zelenko added inline comments.Jan 4 2020, 10:07 PM
clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
47

Please separate with empty line.

Oh, thank you for working on this one, i've always missed this particular check.

Given that there's finally progress on const-correctness check,
should this only handle adding */&, and leave const alone?

May be check belong to LLVM module?

I wouldn't say this is really llvm-specific.

Nice to see this patch! I intended to look at a similar patch, but that's no longer required.

One question does it also handle cases like std::vector<int **> PtrPtrVec; properly?

clang-tools-extra/test/clang-tidy/checkers/readability-qualified-auto.cpp
20

Wouldn't it be nicer to write 'auto NakedPtr' can be declared as an 'auto *NakedPtr' ?
(Likewise for the other warnings.)

24

I looks like the fix adds an extra space character after the * in auto * NakedCPtr

28

I looks like the fix adds an extra space character, giving two spaces in const ConstPtr

I'll address those issues tonight. As for the ** case, I'll. Hadn't even thought of that, I'll try and sort that out

side note when creating this, the add_new_check.py file hasn't been updated in relation to this commit https://github.com/llvm/llvm-project/commit/d2c9c9157b0528436d3b9f5e22c872f0ee6509a2. This results in a malformed rst file. In this patch I have just manually set it up correctly, but a fix will need to be made for the add_new_check.py

My bad, sorry, I will have a look!

+1 for the check from me as well :)

clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
20

Please make the comments full sentences with punctuation.

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

49

thats dangerous. Your source location could be invalid or a macroID, I would rather make these llvm::Optional-

62

the outer const might be debatable. some code-bases don't want the * const, i think neither does LLVM. maybe that could be configurable?

72

please make that comment a full sentence with punctuation and ellide the braces.

80

braces.

88

braces.

97

full sentence.

105

braces.

clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst
46

I think this justifies an alias into the llvm-module we have.

clang-tools-extra/test/clang-tidy/checkers/readability-qualified-auto.cpp
54

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.

101

What about

auto my_function -> int * { /* foo */ }

other contexts for auto need to be considered (lambdas, others?)

njames93 updated this revision to Diff 236255.Jan 5 2020, 11:23 AM
njames93 marked 13 inline comments as done.

fixed some of the code guidelines issues. Will tackle some of the more pressing issues like ensuring correctness

njames93 updated this revision to Diff 236486.Jan 6 2020, 5:34 PM

I have tried to make the code more robust against macro decls. Also target the actual type specifier rather than everything before the name when doing the replacements. This should leave any other qualifiers or attributes in tact. The test cases have been updated to add checks for using and typedefs alias. There are also tests for function pointers and lambdas, with the current behaviour to add pointer qual for function pointer but not for lambdas(as they aren't function pointers under the hood). Not sure I want to implement support for pointers to pointers as that's not going to help readability(Are you a pointer to an array of arrays, array of pointers, pointer to array or pointer to pointer). As for the alias whats the correct procedure for creating an alias

njames93 marked 7 inline comments as done.Jan 7 2020, 7:21 AM

What do you think about volatile qualifiers. Personally I don't think its important to qualify an volatile on a pointer that is volatile, but i should adhere to the decl being declared as volatile

volatile auto X = getPointer();
//transform to
auto *X volatile = getPointer();

auto X = getVolatilePointer();
//transform to
auto *X = getVolatilePointer();
clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
62

The outer const is only there if the vardecl was already declared as const

const auto X = getPointer();
auto *const X = getPointer();

Those 2 decls are functionally identical, both mean that the pointer X can't change but X is pointing to data that can change

JonasToth added inline comments.Jan 7 2020, 8:27 AM
clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
62

I see. That is ok then, but might be surprising to some users, as they might think the pointee itself is const (which it isn't). I think thats something worth to write in the documentation.

clang-tools-extra/test/clang-tidy/checkers/readability-qualified-auto.cpp
184

How does that beatiful code-construct get handled?

auto * function_ptr = +[](int) {};

The lambda is actually converted to a function pointer through the +-operator. Not sure this happens anywhere, but it can happen.
So the case:

auto unnotice_ptr = +[](int) {};

should be transformed.

njames93 marked an inline comment as done.Jan 7 2020, 8:47 AM

Would you say this behaviour is better?

void baz() {
  auto MyFunctionPtr = getPtrFunction();
  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: 'auto MyFunctionPtr' can be declared as 'auto *MyFunctionPtr'
  // CHECK-FIXES-NOT: {{^}}  auto *MyFunctionPtr = getPtrFunction();
  auto MyFunctionVal = getValFunction();
  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: 'auto MyFunctionVal' can be declared as 'auto *MyFunctionVal'
  // CHECK-FIXES-NOT: {{^}}  auto *MyFunctionVal = getValFunction();

  auto LambdaTest = [] { return 0; };
  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: 'auto LambdaTest' can be declared as 'auto *LambdaTest'
  // CHECK-FIXES-NOT: {{^}}  auto *LambdaTest = [] { return 0; };#

  auto LambdaTest2 = +[] { return 0; };
  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: 'auto LambdaTest2' can be declared as 'auto *LambdaTest2'
  // CHECK-FIXES-NOT: {{^}}  auto *LambdaTest2 = +[] { return 0; };
}
clang-tools-extra/test/clang-tidy/checkers/readability-qualified-auto.cpp
184

It gets handled the same as retty (*)(params), I should probably make function pointers in general not be converted

njames93 updated this revision to Diff 236615.Jan 7 2020, 9:29 AM

adhere to coding guidelines, added check for local volatile qualifiers, disregard pointer addition on function pointer types

njames93 updated this revision to Diff 236617.Jan 7 2020, 9:35 AM

Added docs for const and volatile qualifiers

njames93 marked 2 inline comments as done.Jan 7 2020, 9:36 AM
JonasToth added inline comments.Jan 7 2020, 9:44 AM
clang-tools-extra/test/clang-tidy/checkers/readability-qualified-auto.cpp
184

Ok.
There exist function references, too int (&my_reference) (int); ( i think that was the syntax).
They should then follow this as well (+ user-facing documentation).

njames93 marked 2 inline comments as done.Jan 7 2020, 4:11 PM
njames93 added inline comments.
clang-tools-extra/test/clang-tidy/checkers/readability-qualified-auto.cpp
184

You are right that there is a syntax like that, but if you try to bind one to an auto, it will decay to a pointer. If you bind one to an auto reference, then it will be just like the reference syntax above. In either case this won't trigger any replacements

njames93 updated this revision to Diff 236723.Jan 7 2020, 5:19 PM
njames93 marked an inline comment as done.

Added an alias into the llvm module, LMK if I have done that incorrectly

njames93 marked an inline comment as done.Jan 7 2020, 5:20 PM

By the word, will be interesting to see results of this check run on LLVM code. Probably results should be split on modules.

clang-tools-extra/docs/ReleaseNotes.rst
151

Please move alias entry into aliases section. See previous release for proper order.

clang-tools-extra/docs/clang-tidy/checks/llvm-qualified-auto.rst
7

Please make length same as title.

njames93 updated this revision to Diff 236780.Jan 8 2020, 1:59 AM
njames93 marked 3 inline comments as done.

By the word, will be interesting to see results of this check run on LLVM code. Probably results should be split on modules.

So ran it on clang and clang-tidy just crashed, gonna debug it see whats happening

clang-tools-extra/docs/ReleaseNotes.rst
151

I looked at the previous release notes and from what i can see, all alias checks are next to each other in alphabetical order, but there is no special placement of the alias checks in the release notes, they just start at an arbitrary position.

By the word, will be interesting to see results of this check run on LLVM code. Probably results should be split on modules.

So ran it on clang and clang-tidy just crashed, gonna debug it see whats happening

Crash fixed heres what happened when I ran it on clang/lib

Quite a few occurances of const auto Decl which are renamed as const auto* const, or worse still a few that are redeclared as auto *const

/home/nathan/src/llvm-project/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:623:3: warning: 'const auto IC' can be declared as 'const auto *const IC' [readability-qualified-auto]
  const auto IC = dyn_cast<CXXInstanceCall>(&Call);
  ^~~~~~~~~~~
  const auto *const 
/home/nathan/src/llvm-project/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:636:3: warning: 'const auto MethodDecl' can be declared as 'const auto *const MethodDecl' [readability-qualified-auto]
  const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
  ^~~~~~~~~~~
  const auto *const

286 files changed, without looking at header files. One file failed to build which is due to a dependant template. My guess is one call will have returned a naked pointer, and another returned an iterator, maybe I should disregard dependant templates unless it would be possible to deduce its always a pointer

/home/nathan/src/llvm-project/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:365:9: error: variable 'It' with type 'auto *' has incompatible initializer of type 'typename std::conditional<std::is_const<vector<CheckerInfo, allocator<CheckerInfo> > >::value, typename vector<CheckerInfo, allocator<CheckerInfo> >::const_iterator, typename vector<CheckerInfo, allocator<CheckerInfo> >::iterator>::type' (aka '__gnu_cxx::__normal_iterator<clang::ento::CheckerRegistry::CheckerInfo *, std::vector<clang::ento::CheckerRegistry::CheckerInfo, std::allocator<clang::ento::CheckerRegistry::CheckerInfo> > >')
  auto *It = binaryFind(Collection, FullName);
        ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/nathan/src/llvm-project/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp:378:5: note: in instantiation of function template specialization 'insertOptionToCollection<std::vector<clang::ento::CheckerRegistry::CheckerInfo, std::allocator<clang::ento::CheckerRegistry::CheckerInfo> > >' requested here
    insertOptionToCollection(CheckerOptEntry.first, Checkers,
    ^
1 error generated.

By the word, will be interesting to see results of this check run on LLVM code. Probably results should be split on modules.

So ran it on clang and clang-tidy just crashed, gonna debug it see whats happening

Crash fixed heres what happened when I ran it on clang/lib

Quite a few occurances of const auto Decl which are renamed as const auto* const, or worse still a few that are redeclared as auto *const

/home/nathan/src/llvm-project/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:623:3: warning: 'const auto IC' can be declared as 'const auto *const IC' [readability-qualified-auto]
  const auto IC = dyn_cast<CXXInstanceCall>(&Call);
  ^~~~~~~~~~~
  const auto *const 
/home/nathan/src/llvm-project/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:636:3: warning: 'const auto MethodDecl' can be declared as 'const auto *const MethodDecl' [readability-qualified-auto]
  const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
  ^~~~~~~~~~~
  const auto *const

286 files changed, without looking at header files. One file failed to build which is due to a dependant template. My guess is one call will have returned a naked pointer, and another returned an iterator, maybe I should disregard dependant templates unless it would be possible to deduce its always a pointer

Yes, templates are usually not so nice to handle well :/
One unfortunate thing I am currently looking for is checking if auto->deducedType() is a tempateTypeParmType or substTemplateTypeParmType through matchers, I failed. But in a sense you need to check for that condition for the cases auto -> direct; auto & -> pointee; auto * -> pointee.
From my experience it helps to run it over llvm/lib, too as there is a lot of interesting c++ code in their. My checks broke there more often.

What do you think about volatile qualifiers. Personally I don't think its important to qualify an volatile on a pointer that is volatile, but i should adhere to the decl being declared as volatile

volatile auto X = getPointer();
//transform to
auto *X volatile = getPointer();

auto X = getVolatilePointer();
//transform to
auto *X = getVolatilePointer();

Probably better to stay consistent, especially with volatile not being so obvious.

JonasToth added inline comments.Jan 8 2020, 6:19 AM
clang-tools-extra/docs/clang-tidy/checks/list.rst
402

That line needs to land above where the other LLVM checks are.

clang-tools-extra/test/clang-tidy/checkers/readability-qualified-auto.cpp
2

please split the tests to one that is c++11 (check_clang_tidy should default to that) and one that has only c++17 features.

204

Please add function references as test anyway, even though there is no new behaviour.

Eugene.Zelenko added inline comments.Jan 8 2020, 6:49 AM
clang-tools-extra/docs/ReleaseNotes.rst
151
njames93 updated this revision to Diff 236827.Jan 8 2020, 7:39 AM
njames93 marked 4 inline comments as done.
njames93 marked an inline comment as done.Jan 8 2020, 7:45 AM

By the word, will be interesting to see results of this check run on LLVM code. Probably results should be split on modules.

So ran it on clang and clang-tidy just crashed, gonna debug it see whats happening

Crash fixed heres what happened when I ran it on clang/lib

Quite a few occurances of const auto Decl which are renamed as const auto* const, or worse still a few that are redeclared as auto *const

/home/nathan/src/llvm-project/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:623:3: warning: 'const auto IC' can be declared as 'const auto *const IC' [readability-qualified-auto]
  const auto IC = dyn_cast<CXXInstanceCall>(&Call);
  ^~~~~~~~~~~
  const auto *const 
/home/nathan/src/llvm-project/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp:636:3: warning: 'const auto MethodDecl' can be declared as 'const auto *const MethodDecl' [readability-qualified-auto]
  const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl());
  ^~~~~~~~~~~
  const auto *const

286 files changed, without looking at header files. One file failed to build which is due to a dependant template. My guess is one call will have returned a naked pointer, and another returned an iterator, maybe I should disregard dependant templates unless it would be possible to deduce its always a pointer

Yes, templates are usually not so nice to handle well :/
One unfortunate thing I am currently looking for is checking if auto->deducedType() is a tempateTypeParmType or substTemplateTypeParmType through matchers, I failed. But in a sense you need to check for that condition for the cases auto -> direct; auto & -> pointee; auto * -> pointee.
From my experience it helps to run it over llvm/lib, too as there is a lot of interesting c++ code in their. My checks broke there more often.

I managed to make some headway with templates, but I think I'll need to add some new ast matchers to get full coverage. However this patch is a little big for more AST matchers as well so that can come at a later date. Right now the behaviour is ignore template instantiations apart from function instantiations where the type is deduced as T*. using that I'm able to run across the whole of llvm and clang lib with no errors in compilation

clang-tools-extra/docs/ReleaseNotes.rst
151

Ahh I see, what threw me is there is an alias in release notes in master that is just randomly thrown in the middle, guess I'll change both of them

clang-tools-extra/docs/clang-tidy/checks/list.rst
402

How come, thought this section was just for alias declarations? or do llvm checks not get listed in the alias section

Eugene.Zelenko added inline comments.Jan 8 2020, 7:53 AM
clang-tools-extra/docs/ReleaseNotes.rst
151

I think best solution would be to introduce section headers, otherwise people would make similar mistakes during merges in future.

njames93 marked an inline comment as done.Jan 8 2020, 8:01 AM
njames93 added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
151

Shall I fix the order of everything and add sections then moving forward it should stay consistent?

Eugene.Zelenko added inline comments.Jan 8 2020, 8:12 AM
clang-tools-extra/docs/ReleaseNotes.rst
151

It'll be very reasonable fix, especially because of upcoming release, but probably it should be done in separate patch.

njames93 updated this revision to Diff 236833.Jan 8 2020, 8:18 AM
njames93 marked 4 inline comments as done.

Those pesky new lines

clang-tools-extra/docs/ReleaseNotes.rst
151

I'll leave this as is then

njames93 updated this revision to Diff 236859.Jan 8 2020, 10:34 AM

better template support, still runs on llvm and clang libs just fine

aaron.ballman added inline comments.Jan 8 2020, 11:40 AM
clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
32

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.)

94

You shouldn't use auto here as the type is not spelled out in the initialization.

95–96

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.

102–103

You can skip calling getTypePtr() and just use the -> overload.

104–105

Same observations here as above.

160

auto *

161

Might as well move this closer to where it's actually used.

Eugene.Zelenko added inline comments.Jan 8 2020, 11:49 AM
clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
160

This would be nice test for check.

njames93 marked 10 inline comments as done.Jan 8 2020, 3:08 PM
njames93 added inline comments.
clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
32

I'll add in support for it

95–96

Well it kept crashing without that check so I need to do some more digging on that front

160

I feel like this is what the face-palm emoji is for. I should've pointed that run clang tidy script at clang-tools-extra

njames93 updated this revision to Diff 236946.Jan 8 2020, 6:28 PM
njames93 marked 4 inline comments as done.

Runs on the entire llvm project with no build issues afterwards. also fixed my facepalm

clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
95–96

Figured it out, its when the type deduced from auto is a dependant template type, getting the desugarded type is working atm, but I dont know the ast well enough to be sure thats the correct method

160

that wont happen again now :D Idiot proof

njames93 marked 2 inline comments as done and an inline comment as not done.Jan 8 2020, 6:31 PM
njames93 added inline comments.
clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
95–96

I put in an assert check just for sanity reasons. but I have had no issues now I have updated slightly

njames93 updated this revision to Diff 237034.Jan 9 2020, 5:26 AM

small nits

Please rebase from trunk. I sorted Clang-tidy entries in Release Notes and aliases have own subsubsection. Please keep alphabetical order there.

njames93 updated this revision to Diff 237329.Jan 10 2020, 7:52 AM

Rebase onto trunk

aaron.ballman accepted this revision.Jan 10 2020, 9:09 AM

LGTM aside from nits

clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
84

Can drop the else after return.

100

dyn_cast<> can return null, but you don't check for null here. Should this be using cast<> or should it be checking for null?

170–176

Elide braces. (Same elsewhere in the patch)

clang-tools-extra/docs/clang-tidy/checks/readability-qualified-auto.rst
7

Extra whitespace between variables and that.
auto typed -> auto-typed (with backticks and hyphen)

10

a auto typed -> an auto-typed

This revision is now accepted and ready to land.Jan 10 2020, 9:09 AM
njames93 updated this revision to Diff 237377.Jan 10 2020, 10:13 AM
njames93 marked 4 inline comments as done.

reformat/lint

njames93 updated this revision to Diff 237378.Jan 10 2020, 10:19 AM
njames93 marked 2 inline comments as done.
  • Update docs to use backticks
clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
100

It should be using cast, we already know that its an `AutoType` because the matcher matched on autoType

Do you need someone to commit this on your behalf? (If you plan to continue contributing, which I hope you do, it may be a good time for you to obtain commit privileges: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access but I am happy to commit on your behalf if you'd prefer.)

Do you need someone to commit this on your behalf? (If you plan to continue contributing, which I hope you do, it may be a good time for you to obtain commit privileges: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access but I am happy to commit on your behalf if you'd prefer.)

For now I would appreciate it, I have fired off an email though, cheers.

Do you need someone to commit this on your behalf? (If you plan to continue contributing, which I hope you do, it may be a good time for you to obtain commit privileges: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access but I am happy to commit on your behalf if you'd prefer.)

For now I would appreciate it, I have fired off an email though, cheers.

Happy to do so -- can you upload a rebased patch? I am getting merge conflicts:

c:\llvm-project>git apply C:\Users\aballman\Desktop\D72217.diff
C:/Users/aballman/Desktop/D72217.diff:402: trailing whitespace.
  Adds pointer and ``const`` qualifications to ``auto``-typed variables
C:/Users/aballman/Desktop/D72217.diff:468: trailing whitespace.
make it obvious if a ``auto`` typed variable is a pointer, constant pointer or
C:/Users/aballman/Desktop/D72217.diff:469: trailing whitespace.
constant reference. This check will transform ``auto`` to ``auto *`` when the
error: patch failed: clang-tools-extra/docs/clang-tidy/checks/list.rst:397
error: clang-tools-extra/docs/clang-tidy/checks/list.rst: patch does not apply
njames93 updated this revision to Diff 238036.Jan 14 2020, 10:40 AM
  • Rebase from trunk

Thank you for the new check, I've commit on your behalf in 36fcbb838c8f293f46bfed78c6ed8c177f1e3485

Unit tests: pass. 61851 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

This check as configured for LLVM itself is pretty noisy, generating warnings like:

warning: 'auto *CTSD' can be declared as 'const auto *CTSD' [llvm-qualified-auto]

which the LLVM dev guide doesn't have an opinion about.

AFAICS there's no option to disable just the const behavior, and no proposal to change the dev guidelines - is someone working on something here?
Otherwise I'd like to turn this off at least for clang-tools-extra.

This check as configured for LLVM itself is pretty noisy, generating warnings like:

warning: 'auto *CTSD' can be declared as 'const auto *CTSD' [llvm-qualified-auto]

which the LLVM dev guide doesn't have an opinion about.

AFAICS there's no option to disable just the const behavior, and no proposal to change the dev guidelines - is someone working on something here?
Otherwise I'd like to turn this off at least for clang-tools-extra.

https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto

// Copy pointers, but make it clear that they're pointers.
for (const auto *Ptr : Container) { observe(*Ptr); }
for (auto *Ptr : Container) { Ptr->change(); }

This is the reasoning behind putting the const qualifier in the check. If people feel that this isn't quite enough to justify the const qualifier I'll submit a follow up. The general consensus though is that you should mark auto pointers as const if they don't need to change to express intent

https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto

// Copy pointers, but make it clear that they're pointers.
for (const auto *Ptr : Container) { observe(*Ptr); }
for (auto *Ptr : Container) { Ptr->change(); }

This is the reasoning behind putting the const qualifier in the check. If people feel that this isn't quite enough to justify the const qualifier I'll submit a follow up. The general consensus though is that you should mark auto pointers as const if they don't need to change to express intent

The text/rule there is explicitly about avoiding/clarifying copies - the examples indeed use 'const' but AFAICT the "don't copy" reasoning only applies to including *&.

FWIW I think const here is often noise, particularly in AST-walking code where you're traversing an edge from an X* to a Y* - the latter will be const if the former is, and I care at API boundaries but not in between. (It's usually a meaningless distinction - e.g. we're just reading but it's a non-const pointer because RecursiveASTVisitor isn't const-friendly).

So while spelling const is often helpful, we shouldn't (and don't) require it, and the current config of this check is too intrusive.

The text/rule there is explicitly about avoiding/clarifying copies - the examples indeed use 'const' but AFAICT the "don't copy" reasoning only applies to including *&.

FWIW I think const here is often noise, particularly in AST-walking code where you're traversing an edge from an X* to a Y* - the latter will be const if the former is, and I care at API boundaries but not in between. (It's usually a meaningless distinction - e.g. we're just reading but it's a non-const pointer because RecursiveASTVisitor isn't const-friendly).

So while spelling const is often helpful, we shouldn't (and don't) require it, and the current config of this check is too intrusive.

I have always been a little unsure, a few of the patches I have submitted reviewers have said to add const to auto * or turn auto into const auto * which gave the impression its a guideline. Could add an option to the check to forgo the const qualifier checks though.

https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto

// Copy pointers, but make it clear that they're pointers.
for (const auto *Ptr : Container) { observe(*Ptr); }
for (auto *Ptr : Container) { Ptr->change(); }

This is the reasoning behind putting the const qualifier in the check. If people feel that this isn't quite enough to justify the const qualifier I'll submit a follow up. The general consensus though is that you should mark auto pointers as const if they don't need to change to express intent

The text/rule there is explicitly about avoiding/clarifying copies - the examples indeed use 'const' but AFAICT the "don't copy" reasoning only applies to including *&.

FWIW I think const here is often noise, particularly in AST-walking code where you're traversing an edge from an X* to a Y* - the latter will be const if the former is, and I care at API boundaries but not in between. (It's usually a meaningless distinction - e.g. we're just reading but it's a non-const pointer because RecursiveASTVisitor isn't const-friendly).

So while spelling const is often helpful, we shouldn't (and don't) require it, and the current config of this check is too intrusive.

Oddly enough, there were several long-time reviewers (including myself) who firmly believed we did document this requirement at one point in time, but we couldn't find evidence of it when we started digging harder. We've been giving this review feedback consistently for many years, so "we don't require it" may not be entirely accurate. We may not have written down a requirement for it, but I've *definitely* heard that we should not be deducing qualifiers and I agree with that sentiment. It makes the code harder to understand as a reviewer because there's no way to tell whether code is mutating the pointer/reference or not without further digging (and it changes where code dispatches to, so it's important information for readers of the code to have).

I'm fine making this into an option, but my preference be that the option is on by default for the readability checker. For the LLVM checker, maybe we default the option to off and argue about changing the coding standard.

https://reviews.llvm.org/D73548
Next step is to fire off an email to llvm-dev about adding it to the coding standard :)