[PATCH] D53025: [clang-tidy] implement new check for const return types.

aaron.ballman added a comment. In https://reviews.llvm.org/D53025#1282720, @JonasToth wrote: > Thank you very much for contributing to LLVM/clang-tidy! More patches are > always welcome ;) > > I found one issue we have overseen that should be fixed. Would you be so kind > and do that before

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added a comment. Don't mind my last nit, we are doing this slight modification. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D53025: [clang-tidy] implement new check for const return types.

aaron.ballman closed this revision. aaron.ballman added a comment. In https://reviews.llvm.org/D53025#1282668, @ymandel wrote: > Rebased patch onto trunk. Thanks! I've commit in r345764. If there are any issues with your patch, I'll revert and let you know what's going on. You're also welcome

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added inline comments. Comment at: test/clang-tidy/readability-const-return-type.cpp:6 + +#include + JonasToth wrote: > One nit: system headers are not allowed in clang-tidy tests. We do mock the > required content. Sorry that I overlooked that. > >

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added a comment. Thank you very much for contributing to LLVM/clang-tidy! More patches are always welcome ;) I found one issue we have overseen that should be fixed. Would you be so kind and do that before comitting? Comment at:

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 171985. ymandel marked an inline comment as done. ymandel added a comment. Rebased patch onto trunk. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt

[PATCH] D53025: [clang-tidy] implement new check for const return types.

aaron.ballman added a comment. In https://reviews.llvm.org/D53025#1282423, @ymandel wrote: > Aaron, Jonas, > > Thank you both so much for being fantastic reviewers! This was my first > experience contributing to clang (upstream) and it has been extremely > positive. Congratulations on

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel marked 4 inline comments as done. ymandel added a comment. Aaron, Jonas, Thank you both so much for being fantastic reviewers! This was my first experience contributing to clang (upstream) and it has been extremely positive. In https://reviews.llvm.org/D53025#1282194, @aaron.ballman

[PATCH] D53025: [clang-tidy] implement new check for const return types.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This LGTM, thank you for working on this check! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 ___

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<<

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added a comment. I've renamed the check. I noticed that the script did not rename the header guard macro, though. Please let me know if/where to file a bug. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 ___

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 171900. ymandel added a comment. Rename readability-const-value-return to readability-const-return-type Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added a comment. In https://reviews.llvm.org/D53025#1281097, @aaron.ballman wrote: > I think this is getting really close! One question: would it make more sense > to name this `readability-const-type-return` or > `readability-const-return-type` instead? It's not that the functions

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 171897. ymandel marked 2 inline comments as done. ymandel added a comment. Small tweaks in response to comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added a comment. Am 30.10.18 um 21:47 schrieb Aaron Ballman via Phabricator: > aaron.ballman added a comment. > > I think this is getting really close! One question: would it make more sense > to name this `readability-const-type-return` or > `readability-const-return-type` instead?

[PATCH] D53025: [clang-tidy] implement new check for const return types.

aaron.ballman added a comment. I think this is getting really close! One question: would it make more sense to name this `readability-const-type-return` or `readability-const-return-type` instead? It's not that the functions return a const *value* that's the issue, it's that the declared

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<<

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<<

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 171765. ymandel marked 2 inline comments as done. ymandel added a comment. Add comment to field. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 171764. ymandel marked an inline comment as done. ymandel added a comment. Modify dialog printing to highlight const token. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt

[PATCH] D53025: [clang-tidy] implement new check for const return types.

aaron.ballman added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<<

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel marked 3 inline comments as done. ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 171700. ymandel added a comment. Reworded diagnostic message and corresponding documentation. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt

[PATCH] D53025: [clang-tidy] implement new check for const return types.

aaron.ballman added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<<

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel marked 2 inline comments as done. ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler

[PATCH] D53025: [clang-tidy] implement new check for const return types.

aaron.ballman added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<<

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel marked 2 inline comments as done. ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler

[PATCH] D53025: [clang-tidy] implement new check for const return types.

aaron.ballman added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<<

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<<

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 171293. ymandel marked 4 inline comments as done. ymandel added a comment. Reword comment on the check, for clarity. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt

[PATCH] D53025: [clang-tidy] implement new check for const return types.

aaron.ballman added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:107 +diag(Def->getInnerLocStart(), "return type %0 is 'const'-qualified " + "hindering compiler optimizations") +<<

[PATCH] D53025: [clang-tidy] implement new check for const return types.

Eugene.Zelenko added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:54 + +namespace { + lebedev.ri wrote: > Extend the namespace /\ above, so that function is also covered? Anonymous namespaces are only for class/struct

[PATCH] D53025: [clang-tidy] implement new check for const return types.

lebedev.ri added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:54 + +namespace { + Extend the namespace /\ above, so that function is also covered? Comment at:

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added a comment. Thanks! What's the next step in the process? Does someone need to approve this change? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added a comment. > I'm not sure I understand what you're trying to break in the check. If you're > thinking of whether the code trips up on the lexical issues, I'm pretty sure > that won't apply here. Once the const-ness is hidden behind a template, the > check doesn't try to fix

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added a comment. In https://reviews.llvm.org/D53025#1271367, @JonasToth wrote: > In https://reviews.llvm.org/D53025#1270784, @ymandel wrote: > > > Correction: the code *catches* the trailing return cases, I just couldn't > > find a way to *fix* them; specifically, I was not able to get

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 171105. ymandel added a comment. Formatting & comment tweaks. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ConstValueReturnCheck.cpp

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 171103. ymandel added a comment. Fix bug in const-token finding and add tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ConstValueReturnCheck.cpp

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added a comment. > Also, the template version (whether trailing or normal return) does not > trigger the matchers at all, from which I surmise that clang doesn't consider > the type const qualified when it is not materialized with an actual type. > I've noted as much in the comments,

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added a comment. In https://reviews.llvm.org/D53025#1270784, @ymandel wrote: > Correction: the code *catches* the trailing return cases, I just couldn't > find a way to *fix* them; specifically, I was not able to get the necessary > source ranges to re-lex the trailing return type.

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added a comment. Correction: the code *catches* the trailing return cases, I just couldn't find a way to *fix* them; specifically, I was not able to get the necessary source ranges to re-lex the trailing return type. Repository: rCTE Clang Tools Extra

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 170423. ymandel marked an inline comment as done. ymandel added a comment. Fix some message comments in tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel marked an inline comment as done. ymandel added inline comments. Comment at: test/clang-tidy/readability-const-value-return.cpp:174 +int **const * n_multiple_ptr(); +int *const & n_pointer_ref(); aaron.ballman wrote: > I'd like to see some more complex

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 170418. ymandel added a comment. Expanded test cases. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ConstValueReturnCheck.cpp

[PATCH] D53025: [clang-tidy] implement new check for const return types.

aaron.ballman added inline comments. Comment at: test/clang-tidy/readability-const-value-return.cpp:174 +int **const * n_multiple_ptr(); +int *const & n_pointer_ref(); I'd like to see some more complex examples, like: ``` int const foo(); int const * const

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added a comment. Thank you for the restructurings, i think the code is now way clearer and the check close to being done (at least from my side :)). Could you please mark all notes you consider done as done? Right now i am a bit lost on what to track on, as the locations of the notes

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:93 +Diagnostics << FixItHint::CreateRemoval( +CharSourceRange::getTokenRange(*PrevLoc, *PrevLoc)); + } else { JonasToth wrote: > ymandel wrote: >

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 169941. ymandel added a comment. Changed check result into struct (was pair). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ConstValueReturnCheck.cpp

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 169938. ymandel marked 5 inline comments as done. ymandel added a comment. Refactors generation of clang-tidy fixits and notes. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:64 + + // Fix the definition. + llvm::Optional Loc = findConstToRemove(Def, Result); JonasToth wrote: > ymandel wrote: > > JonasToth wrote: > > > ymandel wrote: > > > >

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:64 + + // Fix the definition. + llvm::Optional Loc = findConstToRemove(Def, Result); ymandel wrote: > JonasToth wrote: > > ymandel wrote: > > > JonasToth wrote: > > >

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:64 + + // Fix the definition. + llvm::Optional Loc = findConstToRemove(Def, Result); JonasToth wrote: > ymandel wrote: > > JonasToth wrote: > > > I feel that this

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:25 + +// Finds the location of the relevant "const" token in `Def`. +llvm::Optional findConstToRemove( ymandel wrote: > JonasToth wrote: > > ymandel wrote: > > >

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel marked 5 inline comments as done. ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:60 +diag(Def->getInnerLocStart(), + "return type is 'const'-qualifed (possibly behind a type alias), " + "which hinders

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:25 + +// Finds the location of the relevant "const" token in `Def`. +llvm::Optional findConstToRemove( JonasToth wrote: > ymandel wrote: > > JonasToth wrote: > > >

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 169461. ymandel marked 3 inline comments as done. ymandel added a comment. Adjusted reporting of warnings. Also, addressed other comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added inline comments. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:25 + +// Finds the location of the relevant "const" token in `Def`. +llvm::Optional findConstToRemove( ymandel wrote: > JonasToth wrote: > > s/"const"/`const` > here

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel added a comment. Thank you for the detailed comments! They were quite helpful, especially as this is my first llvm/clang review. Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:25 + +// Finds the location of the relevant "const" token in `Def`.

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 169290. ymandel marked 11 inline comments as done. ymandel added a comment. Comment fix. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ConstValueReturnCheck.cpp

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 169282. ymandel added a comment. Spacing fix. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ConstValueReturnCheck.cpp

[PATCH] D53025: [clang-tidy] implement new check for const return types.

Eugene.Zelenko added inline comments. Comment at: docs/clang-tidy/checks/readability-const-value-return.rst:7 +Checks for functions with a ``const``-qualified return type and recommends +removal of the ``const`` keyword. Such use of ``const`` is superfluous, and +prevents

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 169280. ymandel added a comment. comment tweak Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ConstValueReturnCheck.cpp

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 169277. ymandel added a comment. Adusted code in respone to comments. Major differences are: - more warning in situations where a FixIt can't be suggested. - more tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files:

[PATCH] D53025: [clang-tidy] implement new check for const return types.

JonasToth added a comment. Hi ymandel, welcome to the LLVM community and thank you very much for working on that check! If you have any questions or other issues don't hesitate to ask ;) Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:25 + +// Finds the location

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel marked 3 inline comments as done. ymandel added a comment. Thanks for the review. I've addressed the comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 168858. ymandel added a comment. Add missing const. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ConstValueReturnCheck.cpp

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 168857. ymandel marked 2 inline comments as done. ymandel added a comment. Dropped unneeded clang qualifier. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel updated this revision to Diff 168856. ymandel added a comment. Minor changes addressing comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53025 Files: clang-tidy/readability/CMakeLists.txt clang-tidy/readability/ConstValueReturnCheck.cpp

[PATCH] D53025: [clang-tidy] implement new check for const return types.

Eugene.Zelenko added a comment. GCC has -Wignored-qualifiers for long time, so may be better to implement it in Clang? Comment at: clang-tidy/readability/ConstValueReturnCheck.cpp:66 + llvm::Optional Loc = findConstToRemove(Def, Result); + if (!Loc) return; +

[PATCH] D53025: [clang-tidy] implement new check for const return types.

ymandel created this revision. ymandel added reviewers: aaron.ballman, JonasToth, ioeric. Herald added subscribers: cfe-commits, xazax.hun, mgorny. Adds a new check readability-const-value-return, which checks for functions with a ``const``-qualified return type and recommends removal of the