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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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 the necessary source ranges to re-lex the trailing return type.
>
>
> There is `fuchsia/TrailingReturnCheck.cpp` which you look if there is something, I am currently not aware of other checks that handle trailing return types in any way.
>  In principle it is ok to leave these for future improvements, but should be noted as limitation in the docs.


Thanks. Unfortuantely, that code doesn't have what I want.  I've updated the .rst doc to mention this shortcoming.   I'm happy to come back in the future and add support if I can get it figured out.  However, I'd rather not black on this at the moment, if that's ok.

In https://reviews.llvm.org/D53025#1271388, @JonasToth wrote:

> > 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, but please correct me if I've misunderstood.
>
> If the template is not instantiated no information on `const` exists and
>  that might vary between instantiations anyway (if you don't explicitly
>  write `const T`). It would be an interesting test to try and break the
>  check with an explicit `const` template and instantiaions with `const`.
>  (e.g. `template <typename T> const T my_function();` and an
>  instantiation like `gsl::span<const int>`)


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 it; so, lexical issues don't come into play.  It boils down to the whether the matcher for const-ness is implemented correctly.  But, I did add a new test based on this:
`template <typename T> const T p32(T t) { return t; }`
which *is* detected and fixed.  Let me know, though, if you want something more elaborate.  Also, see below, where I *did* find a bug in a related kind of type definition.

>> Finally, as for triggering: I ran this over Google's C++ codebase and found quite a few hits (10k < X < 100k;  not sure I'm allowed to specify a more precise value for X, but if you need it,  let me know and I'll ask).  I sampled 100 of them and confirmed that all were correct.
>> 
>> I also looked at LLVM and found ~130 hits. I sampled 10 and again found them all correct. I'm happy to post all of the llvm hits if you think that would be helpful.
> 
> Did you try the transformation and is it correct (does not break code)?
>  Enough to check on LLVM :)

Good call -- found bugs this way in one of the files, which had two errors of the sort you were asking about above:
https://reviews.llvm.org/source/test-suite/browse/test-suite/trunk/SingleSource/Benchmarks/Misc-C%2B%2B-EH/spirit.cpp;344155$2204
https://reviews.llvm.org/source/test-suite/browse/test-suite/trunk/SingleSource/Benchmarks/Misc-C%2B%2B-EH/spirit.cpp;344155$3133

I've modified the code that finds the const token to fix this bug.  In the process, I simplified it and (I think) found a more general solution to the problem.  I also noticed that this same bug exists in the  AvoidConstParamsInDecls check, so I plan to send a follow up change that fixes that check by having it use the newly introduced `getConstQualifyingToken` function.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53025