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

[ymandel]

Summary

Adds a new check readability-const-value-return, which checks for functions with
a const-qualified return type and recommends removal of the const keyword.
Such use of const is superfluous, and prevents valuable compiler
optimizations.

Based in large part on work done by Justin A. Middleton, summer 2018 and in small part on the (abandoned) review https://reviews.llvm.org/D33531.

[Eugene.Zelenko]

GCC has -Wignored-qualifiers for long time, so may be better to implement it in Clang?

clang-tidy/readability/ConstValueReturnCheck.cpp

67

Please split in two lines.

75

Please don't use auto, because type is not deducible from statement and it's not iterator over container.

docs/ReleaseNotes.rst

60

Please use alphabetical order for list of new checks.

64

Please enclose const in ``.

Once sentence is enough for Release Notes.

docs/clang-tidy/checks/readability-const-value-return.rst

8

Please enclose const in ``.

[ymandel]

Minor changes addressing comments.

[ymandel]

Dropped unneeded clang qualifier.

[ymandel]

Add missing const.

[ymandel]

Thanks for the review. I've addressed the comments.

[JonasToth]

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

clang-tidy/readability/ConstValueReturnCheck.cpp

26

s/"const"/const

27

Please use a static function instead of a anonymous namespace. These are only used for local classes.

38

Please return None instead, same below

61

Please ellide the braces

65

I feel that this comment doesn't add value. Could you please either make it more expressive or remove it?

66

This check does not produce diagnostics if something in the lexing went wrong.
I think even if its not possible to do transformation the warning could still be emitted (at functionDecl location). What do you think?

72

please shorten that warning a bit and use 'const'.

Maybe 'const'-qualified return values hinder compiler optimizations or something in this direction?

80

The const is not common in llvm-code. Please use it only for references and pointers.

What do you think about emitting a note if this transformation can not be done? It is relevant for the user as he might need to do manual fix-up. It would complicate the code as there can only be one(?) diagnostic at the same time (90% sure only tbh).

docs/clang-tidy/checks/list.rst

12

spurious change

docs/clang-tidy/checks/readability-const-value-return.rst

20

please remove the double space

test/clang-tidy/Inputs/readability-const-value-return/macro-def.h

2

You can define these macros directly in the test-case, or do you intend something special? Self-contained tests are prefered.

test/clang-tidy/readability-const-value-return.cpp

5

Please add tests, were the const is hidden behind typedefs/using.

I feel that there should be more test that try to break the lexing part as well.
E.g. const /** I am a comment */ /* weird*/ int function(); be creative here ;)

Could you please add a test-case int ** const * multiple_ptr(); and int *const & pointer_ref();

54

Missing warning?

[ymandel]

Adusted code in respone to comments.

Major differences are:

• more warning in situations where a FixIt can't be suggested.
• more tests.

[ymandel]

comment tweak

[Eugene.Zelenko]

docs/clang-tidy/checks/readability-const-value-return.rst

8

Please remove the double space

[ymandel]

Spacing fix.

[ymandel]

Comment fix.

[ymandel]

Thank you for the detailed comments! They were quite helpful, especially as this is my first llvm/clang review.

clang-tidy/readability/ConstValueReturnCheck.cpp

26

here and throughout. All comments mention const without quotes.

61

Actually, i expanded this to include a diag() statement, so kept the braces.

65

Agreed. I merged the comment below into this one, so one comment describes the rest of the control flow in this block.

66

Nice. I like this.

80

Not the most elegant, but I don't see any other way to display multiple diagnoses. Let me know what you think.

docs/clang-tidy/checks/list.rst

12

right. this was done by the script, so I wonder why it reordered these.

test/clang-tidy/Inputs/readability-const-value-return/macro-def.h

2

I added a comment explaining. Does that justify its existence? If not, I'm fine getting rid of this header.

test/clang-tidy/readability-const-value-return.cpp

54

No, but this is subtle, so added a comment.

[JonasToth]

clang-tidy/readability/ConstValueReturnCheck.cpp

26

I meant that you use the \` thingie to mark const :)
Its better to mark language constructs in the comments as well for better clarity whats meant. Comments need to be full sentences and super clear, otherwise the comment does more harm then helping :)

57

Please make the second sentence a gramatically correct english sentence, right now it's a little off.

60

This diagnostic is nice, but including the type would make it even better. This will resolve typedefs as well (like "'uint32_t (a.k.a. 'unsigned int')").

Necessary code (more info in the cfe-internals manual)

diag(Location, "return type %0 is 'const'-qualified hindering compiler optimizations") << Def->getReturnType();

The value passed in with << must be a QualType.
Also diagnostics don't have punctuation and are not full sentences and should be as short as possible (but still clear).

69

This duplicates the diagnostic above. The diagnostics engine would resolve macro expansion and emit the note: expanded from XXX. I think this does not add value and introduces duplication.

80

What do you want to achieve? I think you just want to append the FixItHint do you?

you can do this with saving the diagnostic object in a variable.

auto Diag = diag(Loc, "Warning Message");

// Foo

if (HaveFix)
    Diag << FixItHint::CreateRemove();

When Diag goes out of scope the diagnostic is actually issued, calling just diag produces a temporary that gets immediately destroyed.

83

You don't need this diagnostic again, you can just use the one and first diagnostic from the beginning. Right now you are creating to many warnings which are all redundant.
Please refactor the logic to

1. Create warning for everything because the matcher found an offending definition
2. If(!NotFixable) return
3. CreateFixOtherwiseForAllDefintions

This looks simpler to me.

93

Twice *PrevLoc?

95

Did you consider diag(Loc, "could not transform this declaration", DiagnosticIDs::Note) which emits a note instead of warning.
You dont need to store these in between.

docs/clang-tidy/checks/list.rst

12

It reads all checks, orders and inserts the new one and prints them out again, thats why the change happened.

test/clang-tidy/Inputs/readability-const-value-return/macro-def.h

2

This test is not necessary. If that would not work, the unit tests for the frontend need to fail. The inclusions and everything are assumed to be correctly done. clang-tidy itself only works on the translation units (meaning the .cpp file and all included headers in one blob). You can remove this test safely.

test/clang-tidy/check_clang_tidy.py

110

please remove these changes once the fix for that is committed upstream.

test/clang-tidy/readability-const-value-return.cpp

54

I was searching for it, but i overlooked the definition obviously! Do you have a test-case where the definition in not part of the translation unit?

One could split the implementation of a class into mutliple .cpp files and then link them together.
For such a case it might be reasonable to not emit the warning for the declaration as there needs to be a definition in the project somewhere. And not emitting the warning removes noise from third party libraries that are just used where only the headers are included.

[ymandel]

Adjusted reporting of warnings.

Also, addressed other comments.

clang-tidy/readability/ConstValueReturnCheck.cpp

26

I meant that you use the \` thingie to mark const :)

Ah. :) Done now for real.

Its better to mark language constructs in the comments as well for better clarity whats meant. Comments need to be full sentences and super clear, otherwise the comment does more harm then helping :)

Is there a specific issue w/ this comment, or are you remarking in general? I've rewritten with the goal or better clarity, but wasn't sure of your intent wrt full sentences.

80

I was responding to your original suggestion to emit a note if the transformation can not be done; I changed the code to allow multiple diagnostics via scoping, but found it less than elegant.

93

Is there a better alternative? I thought that, since token ranges closed on both ends, this constructs a SourceRange that spans the single token at PrevLoc. Instead, I could rework getConstTokLocations to return the actual tokens instead, and then create CharSourceRanges from Tok.getLocation(), Tok.getLastLocation().

95

Yes, not sure what I was thinking there (storing the same string repeatedly). Done, thanks.

test/clang-tidy/Inputs/readability-const-value-return/macro-def.h

2

Sure. Will do. Just to explain a bit: my concern was whether my code properly recognizes that the source location at the macro use comes from a macro and I thought that the cross-file case might be reflected differently in source locs than the in-file case. But, now that I think about it, I can't see any interesting difference.

test/clang-tidy/readability-const-value-return.cpp

54

Yes, n14, below (https://reviews.llvm.org/D53025?id=169290 line 183). I also added a comment there explaining its purpose.

Sorry, I don't follow. Currently, we *don't* warn on declarations unless a) the definition is in the TU and b) something gets in the way of removing the const. So, in the situation you're describing (multiple .cpp files), the declaration will be ignored. Are you explaining why you *agree* with current behavior or are you suggesting a change?

[ymandel]

clang-tidy/readability/ConstValueReturnCheck.cpp

60

Thanks, this is a significant improvement.

[JonasToth]

clang-tidy/readability/ConstValueReturnCheck.cpp

26

The comment is ok with a nit: ... source like when the (drop the comma)

65

Did I tell you the "only one diagnostic in-flight" thing? :D
I told you wrong stuff as you already figured out in the code. Please adjust this comment and the additional scope is not necessary too.

80

Ok. That multiple diagnostics thing is incorrect, please pretend it was never written ;)

91

You dont even need to store the location, you can just emit the diag here :)

93

The Fixits work with sourcelocations, you dont need to get the tokenrange and CharSourceRange things. This looks more complicated then necessary to me. Your findConstToRemove can return a Optional<SourceRange> that encloses the const token. This range can then be used for the FixIts. I think this would make the code a bit clearer as well.

clang-tidy/utils/LexerUtils.cpp

34

You can transform this into std::vector<SourceRange> to group the ranges, Token knows where it ends as well (Token.getEndLoc())

41

I think this function can be simplified as the manual lexing stuff seems unnecessary.
Take a look at https://reviews.llvm.org/D51949#change-B_4XlTym3KPw that uses clang::Lexer functionality quite a bit to do manual lexing.

You can use the public static methods from clang::Lexer to simplify this function.

test/clang-tidy/readability-const-value-return.cpp

54

I agree with the current implementation! After reading it again: it was writing while thinking about the issue ;)

[ymandel]

clang-tidy/readability/ConstValueReturnCheck.cpp

65

But, I think you are *correct* in this assertion. When I tried multiple in-flight diagnostics, it failed with error:

clang-tidy: /usr/local/google/home/yitzhakm/Projects/llvm/llvm/tools/clang/include/clang/Basic/Diagnostic.h:1297: clang::DiagnosticBuilder clang::DiagnosticsEngine::Report(clang::SourceLocation, unsigned int): Assertion `CurDiagID == std::numeric_limits<unsigned>::max() && "Multiple diagnostics in flight at once!"' failed.

Am I doing something wrong?

[JonasToth]

clang-tidy/readability/ConstValueReturnCheck.cpp

65

Then let me revert what I said and claim the first thing again :D

I think, the issue is, that you have the auto Diag = diag() object not destroyed before the next one is created. So temporarily storing the locations for the problematic transformations might be necessary to close the scope of the Diag first and then emit the notes.

It would be a good idea, to make a function that returns you a list of FixIts and the list of problematic transformations.
Having these 2 lists (best is probably llvm::SmallVector<FixItHint, 4>, see https://llvm.org/docs/ProgrammersManual.html#dss-smallvector) simplifies creating the diagnostics a lot.
Then you have 2 scopes for emitting, one scope for the actual warning and replacement and the second scope for emitting the fail-notes.

These 2 scopes could even be methods (necessary to access diag()).

[ymandel]

clang-tidy/readability/ConstValueReturnCheck.cpp

65

Sounds good. Here's a stab at this restructuring:
https://reviews.llvm.org/differential/diff/169718/

Doesn't seem worth factoring the actual diag() calls into methods, but let me know what you think.

I'll go ahead with other changes as well, just wanted to get this out there...

[ymandel]

Refactors generation of clang-tidy fixits and notes.

[ymandel]

Changed check result into struct (was pair).

[ymandel]

clang-tidy/readability/ConstValueReturnCheck.cpp

93

After reworking to pipe the Token all the way through to the use, I ended up sticking with CharSourceRange::getCharRange(), because otherwise the fixithint will convert a plain SourceRange into a token range, which is not what we want (since we already have the lower-level char range from the lexed token).

clang-tidy/utils/LexerUtils.cpp

41

(FWIW, this code was taken (almost directly) from clang-tidy/readability/AvoidConstParamsInDecls.cpp.)

I agree that would potentially simplify the code here, but I'm not sure its the right thing to do. Basically all of the relevant functions create a clang::Lexer internally and do a bunch more work. So, it seems like this case, where we are incrementally proceeding through a source, token by token, we really should be using a (stateful) clang::Lexer directly, rather than repeatedly calling into the static methods and creating/destroying a Lexer with each such call.

[JonasToth]

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 are not where they were as well.

As second thing: Could you please run this check of real world projects? E.g. LLVM but maybe others projects you might work on as well and see if you find false-postives, bugs or the like.

[aaron.ballman]

test/clang-tidy/readability-const-value-return.cpp

175

I'd like to see some more complex examples, like:

int const foo();
int const * const foo();

std::add_const<int> foo();

template <typename Ty>
std::add_const<Ty> foo();

auto foo() -> const int;
auto foo() -> int const;

template <typename Ty>
auto foo() -> std::add_const<Ty>;

I'm also very curious to hear how often this triggers on large code bases, especially ones that claim decent const-correctness.

[ymandel]

Expanded test cases.

[ymandel]

test/clang-tidy/readability-const-value-return.cpp

175

I've added examples along the lines you suggested. However, the code cannot currently catch the "auto foo() -> type" form, and I couldn't find an easy way to change this. I've noted as much in the comments.

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.

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.

[ymandel]

Fix some message comments in tests.

[ymandel]

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.

[JonasToth]

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

[JonasToth]

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

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

[ymandel]

Fix bug in const-token finding and add tests.

[ymandel]

Formatting & comment tweaks.

[ymandel]

In D53025#1271367, @JonasToth wrote:

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

[JonasToth]

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.

Yup, just tried to hammer with man possible const-applications.

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.

Perfect!

[ymandel]

Thanks! What's the next step in the process? Does someone need to approve this change?

[lebedev.ri]

clang-tidy/readability/ConstValueReturnCheck.cpp

55

Extend the namespace /\ above, so that function is also covered?

clang-tidy/readability/ConstValueReturnCheck.h

20–21

It isn't immediately clear whether this is talking about

const T func();

or

T func() const; // member function

I'd suggest to emphasize that this is about the const on the return type, not on the function.

[Eugene.Zelenko]

clang-tidy/readability/ConstValueReturnCheck.cpp

55

Anonymous namespaces are only for class/struct declarations. See LLVM coding guidelines.

[aaron.ballman]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

It seems strange to me that this is in the readability module but the diagnostic here is about compiler optimizations. Should this be in the performance module instead? Or should this diagnostic be reworded about the readability concerns?

[ymandel]

Reword comment on the check, for clarity.

[ymandel]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

Good point. The readability angle is that the const clutters the code to no benefit. That is, even if it was performance neutral, I'd still want to discourage this practice. But, it does seem like the primary impact is performance.

I'm fine either way -- moving it to performance or emphasizing the clutter of the unhelpful "const". I'm inclined to moving it, but don't have good sense of how strict these hierarchies are. What do you think?

[aaron.ballman]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

I'm not sold that const-qualified return types always pessimize optimizations. However, I'm also not sold that it's *always* a bad idea to have a top-level cv-qualifier on a return type either (for instance, this is one way to prevent callers from modifying a temp returned by a function). How about leaving this in readability and changing the diagnostic into: "top-level 'const' qualifier on a return type may reduce code readability with limited benefit" or something equally weasely?

[ymandel]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

I talked this over with other google folks and seems like the consensus is:

1. The readability benefits may be controversial. Some folks might not view const as clutter and there are some corner cases where the qualifier may prevent something harmful.
2. The performance implication is real, if not guaranteed to be a problem.

Based on this, seems best to move it to performance, but water down the performance claims. That keeps the focus to an issue we can assume nearly everyone agrees on.

[aaron.ballman]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

I'm not convinced the performance implications are real compared to how the check is currently implemented. I know there are performance concerns when you return a const value of class type because it pessimizes the ability to use move constructors, but that's a very specific performance concern that I don't believe is present in general. For instance, I don't know of any performance concerns regarding const int f(); as a declaration. I'd be fine moving this to the performance module, but I think the check would need to be tightened to only focus on actual performance concerns.

FWIW, I do agree that the readability benefits may be controversial, but I kind of thought that was the point to the check and as such, it's a very reasonable check. Almost everything in readability is subjective to some degree and our metric has always been "if you agree with a style check, don't enable it".

It's up to you how you want to proceed, but I see two ways forward: 1) keep this as a readability check that broadly finds top-level const qualifiers and removes them, 2) move this to the performance module and rework the check to focus on only the scenarios that have concrete performance impact.

[ymandel]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

It's up to you how you want to proceed, but I see two ways forward: 1) keep this as a readability check that broadly finds top-level const qualifiers and removes them, 2) move this to the performance module and rework the check to focus on only the scenarios that have concrete performance impact.

Aaron, thank you for the detailed response. I'm inclined to agree with you that this belongs in readabilty and I spoke with sbenza who feels the same. The high-level point is that the const is noise in most cases.

You suggested above a warning along the lines of:
"top-level 'const' qualifier on a return type may reduce code readability with limited benefit"

I like this, but think it should be a little stronger. Perhaps:
"top-level 'const' qualifier on a return type may reduce code readability while rarely having an effect"

I also propose updating the explanation in the documentation file from:

Such use of `const` is superfluous, and
prevents valuable compiler optimizations.

to the weaker

Such use of `const` is usually superfluous, and can
prevent valuable compiler optimizations.

WDYT?

[aaron.ballman]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

I like this, but think it should be a little stronger. Perhaps:
"top-level 'const' qualifier on a return type may reduce code readability while rarely having an effect"

I'm not opposed to it, but I worry about people getting hung up on "rarely having an effect". For instance, a const return type will definitely have an impact on code using template metaprogramming to inspect the return type of a Callable. How about:

"top-level 'const' qualifier on a return type may reduce code readability without improving const correctness"

Such use of const is usually superfluous, and can
prevent valuable compiler optimizations.

Sounds good to me!

[ymandel]

Reworded diagnostic message and corresponding documentation.

[ymandel]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

"top-level 'const' qualifier on a return type may reduce code readability without improving const correctness"

Sounds good. I merged this into the existing message (so that it still prints the actual type). I don't call out "top level" because I was afraid it would be a mouthful in the context, but I'm happy to put it (back) in if you think it would be helpful to users.

[aaron.ballman]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

I think the "top-level" is important because the diagnostic is otherwise ambiguous. We can fix that in one of two ways: leave the "top-level" wording in there, or highlight the offending const in the type itself. Otherwise, this sort of code would trigger a diagnostic that the user may not be certain of how to resolve: const int * const f();

You already calculate what text to remove, so it may be possible to not use the "top-level" wording and pass in a SourceRange for the const to underline. e.g., diag(FunctionLoc, "message %0") << ConstRange << Def->getReturnType(); This will highlight the function location for the diagnostic, but add underlines under the problematic const in the type.

If that turns out to be a challenge, however, you could also just leave in the "top-level" wording and let the user figure it out from there.

[ymandel]

Modify dialog printing to highlight const token.

[ymandel]

Add comment to field.

[ymandel]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

Ah, I missed that subtlety. Nice. What you suggested worked (on the first try, no less). I like the new version far better, I just hadn't realized it was possible. I've actually taken both suggestions -- mention "top level" and highlight the const token -- because there are cases when we don't have the source range of the const token and I'd like those warnings to be clear as well.

Here's some example output for some of the more complex examples:

warning: return type 'const Clazz::Strukt *const' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-value-return]
const Clazz::Strukt* const Clazz::p7() {}
^ ~~~~~~

warning: return type 'const Klazz<const int> *const' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-value-return]
const Klazz<const int>* const Clazz::p5() const {}
^ ~~~~~~

[JonasToth]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

The cherry on the cake would be, if the ~~~~~~ is shortened by one, to not include the whitespace ;)

[aaron.ballman]

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 return type is top-level const. I think removing "value" and using "type" instead would be an improvement (and similarly, rename the files and the check).

clang-tidy/readability/ConstValueReturnCheck.cpp

112

I think you want getBeginLoc() here instead.

docs/clang-tidy/checks/readability-const-value-return.rst

5

Underlining here is incorrect.

[JonasToth]

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? It's not that the functions return a const *value* that's the issue, it's that the declared return type is top-level const. I think removing "value" and using "type" instead would be an improvement (and similarly, rename the files and the check).

There is a rename-check.py script in the repository. Just to prevent a
lot of manual work ;)

[ymandel]

Small tweaks in response to comments.

[ymandel]

In 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 return a const *value* that's the issue, it's that the declared return type is top-level const. I think removing "value" and using "type" instead would be an improvement (and similarly, rename the files and the check).

Yes, that sounds good. Will do that in a separate diff so you can see my changes in reply to your comments first.

In D53025#1281107, @JonasToth wrote:

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? It's not that the functions return a const *value* that's the issue, it's that the declared return type is top-level const. I think removing "value" and using "type" instead would be an improvement (and similarly, rename the files and the check).

There is a rename-check.py script in the repository. Just to prevent a
lot of manual work ;)

Jonas -- thanks for the tip!

clang-tidy/readability/ConstValueReturnCheck.cpp

108

Agreed! But, I spent a while trying to do this to no avail. I tried these hypotheses:

1. it's an off-by-one issue, e.g. that the SourceRange is sometimes a partially open range of [begin, end) and sometimes [begin, end] and perhaps Token and DiagnosticBuilder are interpreting the range differently. But, that's not it -- AvoidConstParamsInDecls suffers from the same issue, yet when the const appears before a ')' rather than whitespace, the range is correct.

2. it's a whitespace issue -- the token includes all following whitespace. False. It only includes one character of whitespace.

3. the token API provides access to the actual token end. Wrong again (as far as I can tell).

What's left is to hard-code the length of "const" and create the source range based on that. I hate hard coding magic constants, though.

Is there any other way?

112

getInnerLocStart() skips the template declarations, which makes it most likely to point to the beginning of the return type. I figured this would be clearest to the reader. Now that we're highlighting the const token, though, I think this difference is less critical.

I've added a comment explaining, here and above, but let me know if you still want me to change it.

[ymandel]

Rename readability-const-value-return to readability-const-return-type

[ymandel]

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.

[JonasToth]

clang-tidy/readability/ConstValueReturnCheck.cpp

108

Well, it is not an big issue, you don't need to spend a lot of time on that.

I would approach trying to make a SourceRange and then there is a withOffset(-1) on SourceLocation and to construct a range. But really, it's not a big issue :)

[aaron.ballman]

This LGTM, thank you for working on this check!

[ymandel]

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 D53025#1282194, @aaron.ballman wrote:

This LGTM, thank you for working on this check!

Great! I don't have SVN commit access. Can one of you commit this?

clang-tidy/readability/ConstValueReturnCheck.cpp

108

Thanks. Then, I think I'll leave it for future work*. The problem with -1 is that when the token is not followed by whitespace the range is correct. While I don't believe that corner case arises here, I can't be sure and I also don't want a solution that's specific to this case.

*FWIW, my interest in getting involved in clang is to broadly improve interactions with the AST of this sort to make source-to-source rewriting a much easier process. So, I really do plan to come back to this. :)

[aaron.ballman]

In 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 contributing your first patch!

In D53025#1282194, @aaron.ballman wrote:

This LGTM, thank you for working on this check!

Great! I don't have SVN commit access. Can one of you commit this?

I'm happy to do so, but the patch currently does not apply cleanly to the ToT. Can you rebase your patch on trunk and update the review with it?

[ymandel]

Rebased patch onto trunk.

[JonasToth]

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?

test/clang-tidy/readability-const-return-type.cpp

6 ↗

(On Diff #171985)

One nit: system headers are not allowed in clang-tidy tests. We do mock the required content. Sorry that I overlooked that.

You can take the implementation from here: https://en.cppreference.com/w/cpp/types/add_cv

[JonasToth]

test/clang-tidy/readability-const-return-type.cpp

6 ↗

(On Diff #171985)

With that change -- -- -isystem should not be required anymore as well.

[aaron.ballman]

In 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 to join the #llvm IRC channel if you'd like to watch the bots yourself.

[JonasToth]

Don't mind my last nit, we are doing this slight modification.

[aaron.ballman]

In 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 comitting?

Good catch! I didn't see this message before I commit, but I went ahead and implemented the fix in r345766.

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
+  ConstValueReturnCheck.cpp
   ContainerSizeEmptyCheck.cpp
   DeleteNullPointerCheck.cpp
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolConversionCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp

clang-tidy/readability/ConstValueReturnCheck.h

+//===--- ConstValueReturnCheck.h - clang-tidy -------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONSTVALUERETURNCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONSTVALUERETURNCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Suggests removal of the `const` qualifier from any function that returns a
+/// const type.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-const-value-return.html
+class ConstValueReturnCheck : public ClangTidyCheck {
+ public:
+  using ClangTidyCheck::ClangTidyCheck;
+  void registerMatchers(ast_matchers::MatchFinder* finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult& result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONSTVALUERETURNCHECK_H

clang-tidy/readability/ConstValueReturnCheck.cpp

+//===--- ConstValueReturnCheck.cpp - clang-tidy ---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ConstValueReturnCheck.h"
+#include "../utils/LexerUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/Optional.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+// Finds the location of the relevant const token in `Def`.
+static llvm::Optional<SourceLocation> findConstToRemove(
+    const FunctionDecl *Def, const MatchFinder::MatchResult &Result) {
+  // Since either of the locs can be in a macro, use makeFileCharRange to be
+  // sure that we have a consistent char range, located entirely in the source
+  // file.
+  CharSourceRange FileRange = Lexer::makeFileCharRange(
+      CharSourceRange::getCharRange(Def->getBeginLoc(),
+                                    Def->getNameInfo().getLoc()),
+      *Result.SourceManager, Result.Context->getLangOpts());
+
+  if (FileRange.isInvalid())
+    return None;
+
+  std::vector<SourceLocation> Locs = utils::lexer::getConstTokLocations(
+      FileRange, *Result.Context, *Result.SourceManager);
+  if (Locs.empty())
+    return None;
+
+  // If the return type is a pointer, then remove const after  asterisk, which
+  // will be the last const before function name; else, get first.
+  return Def->getReturnType()->isPointerType() ? Locs.back() : Locs.front();
+}
+
+void ConstValueReturnCheck::registerMatchers(MatchFinder *Finder) {
+  // Find all the functions for which the return types are const qualified.
+  Finder->addMatcher(
+      functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+      this);
+}
+
+void ConstValueReturnCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Def = Result.Nodes.getNodeAs<FunctionDecl>("func");
+  if (!Def->getReturnType().isLocalConstQualified()) {
+    // From the matcher, we know this is const qualified.  But it is not done
+    // locally, so we can't offer a fix -- just a warning.
+    diag(Def->getInnerLocStart(),
+         "return type is 'const'-qualifed (possibly behind a type alias), "
+         "which hinders compiler optimizations");
+    return;
+  }
+
+  llvm::Optional<SourceLocation> Loc = findConstToRemove(Def, Result);
+  if (!Loc) {
+    // We couldn't find the const token, most likely because it is hidden behind
+    // a macro. So, warn the user, but offer no fix.
+    diag(Def->getInnerLocStart(),
+         "return type is 'const'-qualifed (possibly behind a macro), which "
+         "hinders compiler optimizations");
+    return;
+  }
+
+  // Limit the scope of Diagnostics object below so that we can emit further
+  // diagnostics after the scope. Clang only supports one in-flight diagnostic
+  // at a time.
+  std::vector<std::pair<clang::SourceLocation, std::string>> UnfixabledDecls;
+  {
+    // Fix the definition and any visible declarations, but don't warn
+    // seperately for each declaration -- associate all fixes with the single
+    // warning at the definition.
+    DiagnosticBuilder Diagnostics =
+        diag(*Loc,
+             "'const'-qualified return values hinder compiler optimizations")
+        << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(*Loc, *Loc));
+
+    for (const FunctionDecl *Decl = Def->getPreviousDecl(); Decl != nullptr;
+         Decl = Decl->getPreviousDecl()) {
+      if (llvm::Optional<SourceLocation> PrevLoc =
+              findConstToRemove(Decl, Result)) {
+        Diagnostics << FixItHint::CreateRemoval(
+            CharSourceRange::getTokenRange(*PrevLoc, *PrevLoc));
+      } else {
+        UnfixabledDecls.emplace_back(
+            Decl->getInnerLocStart(),
+            "definition fixed, but couldn't fix this declaration.");
+      }
+    }
+  }
+  for (auto &d : UnfixabledDecls)
+    diag(d.first, d.second);
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/ReadabilityTidyModule.cpp

 //===--- ReadabilityTidyModule.cpp - clang-tidy ---------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
+#include "ConstValueReturnCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolConversionCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"

 
 class ReadabilityModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidConstParamsInDecls>(
         "readability-avoid-const-params-in-decls");
     CheckFactories.registerCheck<BracesAroundStatementsCheck>(
         "readability-braces-around-statements");
+    CheckFactories.registerCheck<ConstValueReturnCheck>(
+        "readability-const-value-return");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
     CheckFactories.registerCheck<DeleteNullPointerCheck>(
         "readability-delete-null-pointer");
     CheckFactories.registerCheck<DeletedDefaultCheck>(
         "readability-deleted-default");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");

clang-tidy/utils/LexerUtils.h

 namespace tidy {
 namespace utils {
 namespace lexer {
 
 /// Returns previous token or ``tok::unknown`` if not found.
 Token getPreviousToken(SourceLocation Location, const SourceManager &SM,
                        const LangOptions &LangOpts, bool SkipComments = true);
 
+// Gets the start locations of all ``const`` tokens in the Range.
+std::vector<SourceLocation> getConstTokLocations(
+    CharSourceRange Range, const clang::ASTContext &Context,
+    const SourceManager &SM);
+
 } // namespace lexer
 } // namespace utils
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_LEXER_UTILS_H

clang-tidy/utils/LexerUtils.cpp

if (!Lexer::getRawToken(Location, Token, SM, LangOpts) &&
         (!SkipComments || !Token.is(tok::comment))) {
       break;
     }
     Location = Location.getLocWithOffset(-1);
   }
   return Token;
 }
 
+std::vector<SourceLocation> getConstTokLocations(
+    CharSourceRange Range, const clang::ASTContext &Context,
+    const SourceManager &SM) {
+  std::pair<FileID, unsigned> LocInfo =
+      SM.getDecomposedLoc(Range.getBegin());
+  StringRef File = SM.getBufferData(LocInfo.first);
+  const char *TokenBegin = File.data() + LocInfo.second;
+  Lexer RawLexer(SM.getLocForStartOfFile(LocInfo.first), Context.getLangOpts(),
+                 File.begin(), TokenBegin, File.end());
+  Token Tok;
+  std::vector<SourceLocation> Locations;
+  while (!RawLexer.LexFromRawLexer(Tok) &&
+         !SM.isBeforeInTranslationUnit(Range.getEnd(), Tok.getLocation())) {
+    if (Tok.is(tok::raw_identifier)) {
+      IdentifierInfo &Info = Context.Idents.get(
+          StringRef(SM.getCharacterData(Tok.getLocation()), Tok.getLength()));
+      Tok.setIdentifierInfo(&Info);
+      Tok.setKind(Info.getTokenID());
+    }
+    if (Tok.is(tok::kw_const))
+      Locations.push_back(Tok.getLocation());
+  }
+  return Locations;
+}
+
 } // namespace lexer
 } // namespace utils
 } // namespace tidy
 } // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - New :doc:`abseil-duration-division
   <clang-tidy/checks/abseil-duration-division>` check.
 
   Checks for uses of ``absl::Duration`` division that is done in a
   floating-point context, and recommends the use of a function that
   returns a floating-point value.
 
 - New :doc:`abseil-faster-strsplit-delimiter
   <clang-tidy/checks/abseil-faster-strsplit-delimiter>` check.
 
   Finds instances of ``absl::StrSplit()`` or ``absl::MaxSplits()`` where the
   delimiter is a single character string literal and replaces with a character.
 
- New :doc:`modernize-concat-nested-namespaces
   syntax introduced in C++17 standard: ``namespace a::b { ... }``.
   
 - New :doc:`modernize-deprecated-ios-base-aliases
   <clang-tidy/checks/modernize-deprecated-ios-base-aliases>` check.
 
   This check warns the uses of the deprecated member types of ``std::ios_base``
   and replaces those that have a non-deprecated equivalent.
 
+- New :doc:`readability-const-value-return
+  <clang-tidy/checks/readability-const-value-return>` check.
+
+  Checks for functions with a ``const``-qualified return type and recommends
+  removal of the ``const`` keyword.
+
 - New :doc:`readability-magic-numbers
   <clang-tidy/checks/readability-magic-numbers>` check.
 
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    abseil-duration-division
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls
    abseil-string-find-startswith
    abseil-str-cat-append
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat
    android-cloexec-dup
    android-cloexec-epoll-create
    android-cloexec-epoll-create1
    android-cloexec-fopen
.. toctree::
    performance-move-constructor-init
    performance-noexcept-move-constructor
    performance-type-promotion-in-math-fn
    performance-unnecessary-copy-initialization
    performance-unnecessary-value-param
    portability-simd-intrinsics
    readability-avoid-const-params-in-decls
    readability-braces-around-statements
+   readability-const-value-return
    readability-container-size-empty
    readability-delete-null-pointer
    readability-deleted-default
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-conversion
    readability-inconsistent-declaration-parameter-name

docs/clang-tidy/checks/readability-const-value-return.rst

+.. title:: clang-tidy - readability-const-value-return
+
+readability-const-value-return
+===================================
+
+Checks for functions with a ``const``-qualified return type and recommends
+removal of the ``const`` keyword. Such use of ``const`` is superfluous, and
+prevents valuable compiler optimizations.
+
+Examples:
+
+.. code-block:: c++
+    
+  const int foo();
+  const Clazz foo();
+  Clazz *const foo();
+
+Note that this does not apply to returning pointers or references to const
+values. These are fine:
+
+.. code-block:: c++
+
+   const int* foo();
+   const int& foo();
+   const Clazz* foo();

test/clang-tidy/Inputs/readability-const-value-return/macro-def.h

+#ifndef MACRO_DEF_H
+#define MACRO_DEF_H
+
+// Used in testing the interaction between cross-file macro use and the lexer's
+// ability to "see" the const token.
+#define DEFFF const int
+
+#endif // MACRO_DEF_H

test/clang-tidy/check_clang_tidy.py

for check in args.check_suffix:
         sys.exit('Only A..Z, 0..9 and "-" are ' +
           'allowed in check suffixes list, but "%s" was given' % (check))
 
       file_check_suffix = '-' + check
       check_fixes_prefix = 'CHECK-FIXES' + file_check_suffix
       check_messages_prefix = 'CHECK-MESSAGES' + file_check_suffix
       check_notes_prefix = 'CHECK-NOTES' + file_check_suffix
 
-      has_check_fix = check_fixes_prefix in input_text 
+      has_check_fix = check_fixes_prefix in input_text
       has_check_message = check_messages_prefix in input_text
-      has_check_note = check_notes_prefix in input_text 
+      has_check_note = check_notes_prefix in input_text
 
       if has_check_note and has_check_message:
         sys.exit('Please use either %s or %s but not both' %
           (check_notes_prefix, check_messages_prefix))
 
       if not has_check_fix and not has_check_message and not has_check_note:
         sys.exit('%s, %s or %s not found in the input' %
           (check_fixes_prefix, check_messages_prefix, check_notes_prefix))
 
       has_check_fixes = has_check_fixes or has_check_fix
       has_check_messages = has_check_messages or has_check_message
       has_check_notes = has_check_notes or has_check_note
 
       check_fixes_prefixes.append(check_fixes_prefix)
       check_messages_prefixes.append(check_messages_prefix)
       check_notes_prefixes.append(check_notes_prefix)
   else:
     check_fixes_prefixes = ['CHECK-FIXES']
     check_messages_prefixes = ['CHECK-MESSAGES']
     check_notes_prefixes = ['CHECK-NOTES']
+    has_check_fixes = check_fixes_prefixes[0] in input_text
+    has_check_messages = check_messages_prefixes[0] in input_text
+    has_check_notes = check_notes_prefixes[0] in input_text
 
   # Remove the contents of the CHECK lines to avoid CHECKs matching on
   # themselves.  We need to keep the comments to preserve line numbers while
   # avoiding empty lines which could potentially trigger formatting-related
   # checks.
   cleaned_test = re.sub('// *CHECK-[A-Z0-9\-]*:[^\r\n]*', '//', input_text)
 
   write_file(temp_file_name, cleaned_test)
def main():
   except subprocess.CalledProcessError as e:
     diff_output = e.output
 
   print('------------------------------ Fixes -----------------------------\n' +
         diff_output.decode() +
         '\n------------------------------------------------------------------')
 
   if has_check_fixes:
+    print('Checking Fixes')
     try:
       subprocess.check_output(
           ['FileCheck', '-input-file=' + temp_file_name, input_file_name,
            '-check-prefixes=' + ','.join(check_fixes_prefixes),
            '-strict-whitespace'],
           stderr=subprocess.STDOUT)
     except subprocess.CalledProcessError as e:
       print('FileCheck failed:\n' + e.output.decode())

test/clang-tidy/readability-const-value-return.cpp

+// RUN: %check_clang_tidy %s readability-const-value-return %t -- -- -I %S/Inputs/readability-const-value-return
+
+#include "macro-def.h"
+
+//  p# = positive test
+//  n# = negative test
+
+const int p1() {
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: 'const'-qualified return values hinder compiler optimizations
+// CHECK-FIXES: int p1() {
+  return 1;
+}
+
+const int p15();
+// CHECK-FIXES: int p15();
+
+template <typename A> class Klazz {};
+class Clazz {
+ public:
+  Clazz *const p2() {
+    // CHECK-MESSAGES: [[@LINE-1]]:10: warning: 'const'-qualified return values
+    // CHECK-FIXES: Clazz *p2() {
+    return this;
+  }
+
+  Clazz *const p3();
+  // CHECK-FIXES: Clazz *p3();
+
+  const int p4() const {
+    // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'const'-qualified return values h
+    // CHECK-FIXES: int p4() const {
+    return 4;
+  }
+
+  const Klazz<const int>* const p5() const;
+  // CHECK-FIXES: const Klazz<const int>* p5() const;
+
+  const Clazz operator++(int x) {  //  p12
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'const'-qualified return values hin
+  // CHECK-FIXES: Clazz operator++(int x) {
+  }
+
+  struct Strukt {
+    int i;
+  };
+
+  const Strukt p6() {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 'const'-qualified return values hin
+  // CHECK-FIXES: Strukt p6() {}
+
+  // No warning is emitted here, because this is only the declaration.  The
+  // warning will be associated with the definition, below.
+  const Strukt* const p7();
+  // CHECK-FIXES: const Strukt* p7();
+
+  static const int p8() {}
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: 'const'-qualified return values hi
+  // CHECK-FIXES: static int p8() {}
+
+  static const Strukt p9() {}
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: 'const'-qualified return values hi
+  // CHECK-FIXES: static Strukt p9() {}
+
+  int n0() const { return 0; }
+  const Klazz<const int>& n11(const Klazz<const int>) const;
+};
+
+Clazz *const Clazz::p3() {
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: 'const'-qualified return values hin
+  // CHECK-FIXES: Clazz *Clazz::p3() {
+  return this;
+}
+
+const Klazz<const int>* const Clazz::p5() const {}
+// CHECK-MESSAGES: [[@LINE-1]]:25: warning: 'const'-qualified return values hind
+// CHECK-FIXES: const Klazz<const int>* Clazz::p5() const {}
+
+const Clazz::Strukt* const Clazz::p7() {}
+// CHECK-MESSAGES: [[@LINE-1]]:22: warning: 'const'-qualified return values hind
+// CHECK-FIXES: const Clazz::Strukt* Clazz::p7() {}
+
+Clazz *const p10();
+// CHECK-FIXES: Clazz *p10();
+
+Clazz *const p10() {
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: 'const'-qualified return values hin
+  // CHECK-FIXES: Clazz *p10() {
+  return new Clazz();
+}
+
+const Clazz bar;
+const Clazz *const p11() {
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: 'const'-qualified return values hi
+  // CHECK-FIXES: const Clazz *p11() {
+  return &bar;
+}
+
+const Klazz<const int> p12() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: 'const'-qualified return values hinde
+// CHECK-FIXES: Klazz<const int> p12() {}
+
+const Klazz<const int>* const p13() {}
+// CHECK-MESSAGES: [[@LINE-1]]:25: warning: 'const'-qualified return values hind
+// CHECK-FIXES: const Klazz<const int>* p13() {}
+
+const auto p14() {
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: 'const'-qualified return values hind
+// CHECK-FIXES: auto p14() {
+  const int i = 0;
+  return i;
+}
+
+// re-declaration of p15.
+const int p15();
+// CHECK-FIXES: int p15();
+
+const int p15() {
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: 'const'-qualified return values hinde
+// CHECK-FIXES: int p15() {
+  return 0;
+}
+
+// Exercise the lexer.
+
+const /* comment */ /* another comment*/ int p16() { return 0; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: 'const'-qualified return values hinde
+// CHECK-FIXES: /* comment */ /* another comment*/ int p16() { return 0; }
+
+/* comment */ const
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning: 'const'-qualified return values hind
+// CHECK-FIXES: /* comment */
+// more
+/* another comment*/ int p17() { return 0; }
+
+// Test cases where the `const` token lexically is hidden behind some form of
+// indirection.
+
+#define CONSTINT const int
+CONSTINT p18() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type is 'const'-qualifed (possibly behind a macro), which hinders compiler optimizations
+
+#define CONST const
+CONST int p19() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type is 'const'-qualifed (poss
+
+// DEFFF is defined in header macro-def.h.  This tests that macros defined in
+// other files are handled correctly.
+DEFFF p20() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type is 'const'-qualifed (poss
+
+using ty = const int;
+ty p21() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type is 'const'-qualifed (possibly behind a type alias), which hinders compiler optimizations
+
+typedef const int ty2;
+ty2 p22() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type is 'const'-qualifed (poss
+
+// Declaration uses a macro, while definition doesn't.  In this case, we won't
+// fix the declaration, and will instead issue a warning.
+CONST int p23();
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: definition fixed, but couldn't fix this declaration.
+const int p23();
+// CHECK-FIXES: int p23();
+const int p23() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: 'const'-qualified return values hinde
+// CHECK-FIXES: int p23() {}
+
+const int n1 = 1;
+const Clazz n2 = Clazz();
+const Clazz* n3 = new Clazz();
+Clazz *const n4 = new Clazz();
+const Clazz *const n5 = new Clazz();
+constexpr int n6 = 6;
+constexpr int n7() { return 8; }
+const int eight = 8;
+constexpr const int* n8() { return &eight; }
+Klazz<const int> n9();
+const Klazz<const int>* n10();
+const Klazz<const int>& Clazz::n11(const Klazz<const int>) const {}
+
+// Declaration only.
+const int n14();
+
+int **const * n_multiple_ptr();
+int *const & n_pointer_ref();