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

njames93 added a comment. 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

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

njames93 created this revision. njames93 added a reviewer: aaron.ballman. njames93 added projects: clang, clang-tools-extra. Herald added subscribers: xazax.hun, whisperity, mgorny. njames93 added a comment. side note when creating this, the add_new_check.py file hasn't been updated in relation t

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

Eugene.Zelenko added a comment. May be check belong to LLVM module? Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:69 + if (auto Var = Result.Nodes.getNodeAs("auto_ptr")) { + +if (!Var->getType().getTypePtr()->getPointeeType().isConstQualified(

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:46 +} +CharSourceRange getFixitRange(const VarDecl *Var) { + return CharSourceRange::getTokenRange(Var->getBeginLoc(), Please separate with empty li

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

lebedev.ri added a comment. 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? In D72217#1804628

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

Mordante added a comment. 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 PtrPtrVec;` properly? Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-qualifie

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

njames93 added a comment. 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 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217

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

sylvestre.ledru added a comment. In D72217#1804533 , @njames93 wrote: > 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/d2c9c9157b0528436d3b9f5e22c872f

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

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 sent

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

njames93 updated this revision to Diff 236255. njames93 marked 13 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files: clang-tools-extra/clang-tidy/readability/CMakeLists.txt clang-tools-extra/clang-tidy/readability/

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

njames93 added a comment. fixed some of the code guidelines issues. Will tackle some of the more pressing issues like ensuring correctness CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 ___ cfe-com

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

njames93 updated this revision to Diff 236486. njames93 added a comment. 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 i

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

njames93 marked 7 inline comments as done. njames93 added a comment. 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 = getPointe

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

JonasToth added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:61 + Diag << FixItHint::CreateReplacement( + FixItRange, IsPtrConst ? "const auto *const " : "auto *const "); +} else { njames93 wrote:

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

njames93 marked an inline comment as done. njames93 added a comment. 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-FI

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

njames93 updated this revision to Diff 236615. njames93 added a comment. adhere to coding guidelines, added check for local volatile qualifiers, disregard pointer addition on function pointer types CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217

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

njames93 updated this revision to Diff 236617. njames93 added a comment. Added docs for const and volatile qualifiers CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files: clang-tools-extra/clang-tidy/readability/CMakeLists.txt clang-tools-

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

JonasToth added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-qualified-auto.cpp:183 + + auto LambdaTest = [] { return 0; }; + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: 'auto LambdaTest' can be declared as 'auto *LambdaTest' ---

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

njames93 marked 2 inline comments as done. njames93 added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-qualified-auto.cpp:183 + + auto LambdaTest = [] { return 0; }; + // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: 'auto LambdaTest' can

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

njames93 updated this revision to Diff 236723. njames93 marked an inline comment as done. njames93 added a comment. Added an alias into the llvm module, LMK if I have done that incorrectly CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files:

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

Eugene.Zelenko added a comment. By the word, will be interesting to see results of this check run on LLVM code. Probably results should be split on modules. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:146 +- New alias :doc:`llvm-qualified-auto + ` to ---

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

njames93 updated this revision to Diff 236780. njames93 marked 3 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp clang-tools-extra/clang-tidy/readability/CMak

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

njames93 added a comment. In D72217#1809212 , @Eugene.Zelenko wrote: > 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

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

njames93 added a comment. In D72217#1809685 , @njames93 wrote: > In D72217#1809212 , @Eugene.Zelenko > wrote: > > > By the word, will be interesting to see results of this check run on LLVM > > code. Probably resu

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

JonasToth added a comment. In D72217#1809721 , @njames93 wrote: > In D72217#1809685 , @njames93 wrote: > > > In D72217#1809212 , > > @Eugene.Zelenko wrote: > > > > > By the

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

JonasToth added a comment. In D72217#1807941 , @njames93 wrote: > 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

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

JonasToth added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:402 `hicpp-vararg `_, `cppcoreguidelines-pro-type-vararg `_, + `llvm-qualified-auto `_, `readability-qualified-auto `_, "Yes" That line needs to land above wh

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:146 +- New alias :doc:`llvm-qualified-auto + ` to njames93 wrote: > Eugene.Zelenko wrote: > > Please move alias entry into aliases section. See previous release for > >

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

njames93 updated this revision to Diff 236827. njames93 marked 4 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp clang-tools-extra/clang-tidy/readability/CMak

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

njames93 marked an inline comment as done. njames93 added a comment. In D72217#1809887 , @JonasToth wrote: > In D72217#1809721 , @njames93 wrote: > > > In D72217#1809685 , @n

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:146 +- New alias :doc:`llvm-qualified-auto + ` to njames93 wrote: > Eugene.Zelenko wrote: > > njames93 wrote: > > > Eugene.Zelenko wrote: > > > > Please move alias entry

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

njames93 marked an inline comment as done. njames93 added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:146 +- New alias :doc:`llvm-qualified-auto + ` to Eugene.Zelenko wrote: > njames93 wrote: > > Eugene.Zelenko wrote: > > > njames93 wr

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:146 +- New alias :doc:`llvm-qualified-auto + ` to njames93 wrote: > Eugene.Zelenko wrote: > > njames93 wrote: > > > Eugene.Zelenko wrote: > > > > njames93 wrote: > > > >

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

njames93 updated this revision to Diff 236833. njames93 marked 4 inline comments as done. njames93 added a comment. Those pesky new lines CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.c

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

njames93 added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:146 +- New alias :doc:`llvm-qualified-auto + ` to Eugene.Zelenko wrote: > njames93 wrote: > > Eugene.Zelenko wrote: > > > njames93 wrote: > > > > Eugene.Zelenko wrote: > > > >

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

njames93 updated this revision to Diff 236859. njames93 added a comment. better template support, still runs on llvm and clang libs just fine CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files: clang-tools-extra/clang-tidy/llvm/LLVMTidyModu

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

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

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:159 +void QualifiedAutoCheck::check(const MatchFinder::MatchResult &Result) { + if (auto Var = Result.Nodes.getNodeAs("auto")) { +bool IsPtrConst = isPointerCons

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

njames93 marked 10 inline comments as done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:31 + Const, + Volatile // Ignore restrict as not c++ keyword. +}; aaron.ballman wrote: > We still support `re

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

njames93 added inline comments. 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(); -

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

njames93 marked 2 inline comments as done and an inline comment as not done. njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:94-95 + auto Pointee = QType.getTypePtr()->getPointeeType(); + if (Pointee.isNull()) +re

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

njames93 updated this revision to Diff 236946. njames93 marked 4 inline comments as done. njames93 added a comment. Runs on the entire llvm project with no build issues afterwards. also fixed my facepalm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/

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

njames93 updated this revision to Diff 237034. njames93 added a comment. small nits CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp clang-tools-extra/clang-tidy/readability/CMakeLis

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

Eugene.Zelenko added a comment. Please rebase from trunk. I sorted Clang-tidy entries in Release Notes and aliases have own subsubsection. Please keep alphabetical order there. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 _

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

njames93 updated this revision to Diff 237329. njames93 added a comment. Rebase onto trunk Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp cla

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from nits Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:83 +return llvm::None; + } else if (TypeSpecifier.getEnd()

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

njames93 updated this revision to Diff 237377. njames93 marked 4 inline comments as done. njames93 added a comment. reformat/lint Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files: clang-tools-extra/c

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

njames93 updated this revision to Diff 237378. njames93 marked 2 inline comments as done. njames93 added a comment. - Update docs to use backticks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files: cl

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

njames93 added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp:99 + QualType Pointee = + dyn_cast(QType->getPointeeType().getTypePtr())->desugar(); + assert(!Pointee.isNull() && "can't have a null Pointee"); aar

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

aaron.ballman added a comment. 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 o

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

njames93 added a comment. In D72217#1819437 , @aaron.ballman wrote: > 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:

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

aaron.ballman added a comment. In D72217#1819678 , @njames93 wrote: > In D72217#1819437 , @aaron.ballman > wrote: > > > Do you need someone to commit this on your behalf? (If you plan to continue > > contributing,

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

njames93 updated this revision to Diff 238036. njames93 added a comment. - Rebase from trunk Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 Files: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp c

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

aaron.ballman closed this revision. aaron.ballman added a comment. Thank you for the new check, I've commit on your behalf in 36fcbb838c8f293f46bfed78c6ed8c177f1e3485 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61851 tests passed, 0 failed and 781 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon times-circle color=red} clang-format: fail. Please format your changes with clang-format by runnin

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

sammccall added a comment. 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

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

njames93 added a comment. In D72217#1844112 , @sammccall wrote: > 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

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

sammccall added a comment. In D72217#1844204 , @njames93 wrote: > 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) { observ

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

njames93 added a comment. In D72217#1844262 , @sammccall wrote: > 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 thin

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

aaron.ballman added a comment. In D72217#1844262 , @sammccall wrote: > In D72217#1844204 , @njames93 wrote: > > > https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto > > > > // Copy poin

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

njames93 added a comment. https://reviews.llvm.org/D73548 Next step is to fire off an email to llvm-dev about adding it to the coding standard :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72217/new/ https://reviews.llvm.org/D72217 _