Page MenuHomePhabricator

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

Authored by njames93 on Sat, Jan 4, 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

There are a very large number of changes, so older changes are hidden. Show Older Changes
JonasToth added inline comments.Tue, Jan 7, 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.Tue, Jan 7, 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.Tue, Jan 7, 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.Tue, Jan 7, 9:35 AM

Added docs for const and volatile qualifiers

njames93 marked 2 inline comments as done.Tue, Jan 7, 9:36 AM
JonasToth added inline comments.Tue, Jan 7, 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.Tue, Jan 7, 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.Tue, Jan 7, 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.Tue, Jan 7, 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
146

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

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

Please make length same as title.

njames93 updated this revision to Diff 236780.Wed, Jan 8, 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
146

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.Wed, Jan 8, 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.Wed, Jan 8, 6:49 AM
clang-tools-extra/docs/ReleaseNotes.rst
146
njames93 updated this revision to Diff 236827.Wed, Jan 8, 7:39 AM
njames93 marked 4 inline comments as done.
njames93 marked an inline comment as done.Wed, Jan 8, 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
146

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.Wed, Jan 8, 7:53 AM
clang-tools-extra/docs/ReleaseNotes.rst
146

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.Wed, Jan 8, 8:01 AM
njames93 added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
146

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

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

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.Wed, Jan 8, 8:18 AM
njames93 marked 4 inline comments as done.

Those pesky new lines

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

I'll leave this as is then

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

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

aaron.ballman added inline comments.Wed, Jan 8, 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.Wed, Jan 8, 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.Wed, Jan 8, 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.Wed, Jan 8, 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.Wed, Jan 8, 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.Thu, Jan 9, 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.Fri, Jan 10, 7:52 AM

Rebase onto trunk

aaron.ballman accepted this revision.Fri, Jan 10, 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.Fri, Jan 10, 9:09 AM
njames93 updated this revision to Diff 237377.Fri, Jan 10, 10:13 AM
njames93 marked 4 inline comments as done.

reformat/lint

njames93 updated this revision to Diff 237378.Fri, Jan 10, 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.Tue, Jan 14, 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