[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

66 ↗

(On Diff #168819)

Please split in two lines.

74 ↗

(On Diff #168819)

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

docs/ReleaseNotes.rst

70

Please use alphabetical order for list of new checks.

74

Please enclose const in ``.

Once sentence is enough for Release Notes.

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

7 ↗

(On Diff #168819)

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

25 ↗

(On Diff #168858)

s/"const"/const

26 ↗

(On Diff #168858)

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

37 ↗

(On Diff #168858)

Please return None instead, same below

60 ↗

(On Diff #168858)

Please ellide the braces

64 ↗

(On Diff #168858)

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

65 ↗

(On Diff #168858)

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?

71 ↗

(On Diff #168858)

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

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

79 ↗

(On Diff #168858)

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

13

spurious change

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

19 ↗

(On Diff #168858)

please remove the double space

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

1 ↗

(On Diff #168858)

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

4 ↗

(On Diff #168858)

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

53 ↗

(On Diff #168858)

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

7 ↗

(On Diff #169280)

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

25 ↗

(On Diff #168858)

here and throughout. All comments mention const without quotes.

60 ↗

(On Diff #168858)

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

64 ↗

(On Diff #168858)

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

65 ↗

(On Diff #168858)

Nice. I like this.

79 ↗

(On Diff #168858)

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

13

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

1 ↗

(On Diff #168858)

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

53 ↗

(On Diff #168858)

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

[JonasToth]

clang-tidy/readability/ConstValueReturnCheck.cpp

25 ↗

(On Diff #168858)

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

79 ↗

(On Diff #168858)

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.

57 ↗

(On Diff #169290)

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

60 ↗

(On Diff #169290)

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 ↗

(On Diff #169290)

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.

83 ↗

(On Diff #169290)

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 ↗

(On Diff #169290)

Twice *PrevLoc?

95 ↗

(On Diff #169290)

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

13

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

1 ↗

(On Diff #168858)

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 ↗

(On Diff #169290)

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

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

53 ↗

(On Diff #168858)

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

25 ↗

(On Diff #168858)

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.

79 ↗

(On Diff #168858)

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 ↗

(On Diff #169290)

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 ↗

(On Diff #169290)

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

1 ↗

(On Diff #168858)

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

53 ↗

(On Diff #168858)

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 ↗

(On Diff #169290)

Thanks, this is a significant improvement.

[JonasToth]

clang-tidy/readability/ConstValueReturnCheck.cpp

25 ↗

(On Diff #168858)

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

64 ↗

(On Diff #168858)

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.

79 ↗

(On Diff #168858)

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

93 ↗

(On Diff #169290)

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.

90 ↗

(On Diff #169461)

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

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

53 ↗

(On Diff #168858)

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

[ymandel]

clang-tidy/readability/ConstValueReturnCheck.cpp

64 ↗

(On Diff #168858)

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

64 ↗

(On Diff #168858)

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

64 ↗

(On Diff #168858)

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 ↗

(On Diff #169290)

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

174 ↗

(On Diff #169941)

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

174 ↗

(On Diff #169941)

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

54 ↗

(On Diff #171105)

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

clang-tidy/readability/ConstValueReturnCheck.h

19–20 ↗

(On Diff #171105)

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

54 ↗

(On Diff #171105)

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

[aaron.ballman]

clang-tidy/readability/ConstValueReturnCheck.cpp

107 ↗

(On Diff #171105)

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

107 ↗

(On Diff #171105)

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

107 ↗

(On Diff #171105)

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

107 ↗

(On Diff #171105)

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

107 ↗

(On Diff #171105)

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

107 ↗

(On Diff #171105)

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

107 ↗

(On Diff #171105)

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

107 ↗

(On Diff #171105)

"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

107 ↗

(On Diff #171105)

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

107 ↗

(On Diff #171105)

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

107 ↗

(On Diff #171105)

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

111 ↗

(On Diff #171765)

I think you want getBeginLoc() here instead.

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

4 ↗

(On Diff #171765)

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

107 ↗

(On Diff #171105)

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?

111 ↗

(On Diff #171765)

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

107 ↗

(On Diff #171105)

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

107 ↗

(On Diff #171105)

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

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

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
+  ConstReturnTypeCheck.cpp
   ContainerSizeEmptyCheck.cpp
   DeleteNullPointerCheck.cpp
   DeletedDefaultCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolConversionCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp

clang-tidy/readability/ConstReturnTypeCheck.h

+//===--- ConstReturnTypeCheck.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_CONSTRETURNTYPECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_CONSTRETURNTYPECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// For any function whose return type is const-qualified, suggests removal of
+/// the `const` qualifier from that return type.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-const-return-type.html
+class ConstReturnTypeCheck : 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_CONSTRETURNTYPECHECK_H

clang-tidy/readability/ConstReturnTypeCheck.cpp

+//===--- ConstReturnTypeCheck.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 "ConstReturnTypeCheck.h"
+#include "../utils/LexerUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/SourceLocation.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 qualifying `const` token in the `FunctionDecl`'s
+// return type. Returns `None` when the return type is not `const`-qualified or
+// `const` does not appear in `Def`'s source like when the type is an alias or a
+// macro.
+static llvm::Optional<Token>
+findConstToRemove(const FunctionDecl *Def,
+                  const MatchFinder::MatchResult &Result) {
+  if (!Def->getReturnType().isLocalConstQualified())
+    return None;
+
+  // Get the begin location for the function name, including any qualifiers
+  // written in the source (for out-of-line declarations). A FunctionDecl's
+  // "location" is the start of its name, so, when the name is unqualified, we
+  // use `getLocation()`.
+  SourceLocation NameBeginLoc = Def->getQualifier()
+                                    ? Def->getQualifierLoc().getBeginLoc()
+                                    : Def->getLocation();
+  // Since either of the locs can be in a macro, use `makeFileCharRange` to be
+  // sure that we have a consistent `CharSourceRange`, located entirely in the
+  // source file.
+  CharSourceRange FileRange = Lexer::makeFileCharRange(
+      CharSourceRange::getCharRange(Def->getBeginLoc(), NameBeginLoc),
+      *Result.SourceManager, Result.Context->getLangOpts());
+
+  if (FileRange.isInvalid())
+    return None;
+
+  return utils::lexer::getConstQualifyingToken(FileRange, *Result.Context,
+                                               *Result.SourceManager);
+}
+
+namespace {
+
+struct CheckResult {
+  // Source range of the relevant `const` token in the definition being checked.
+  CharSourceRange ConstRange;
+
+  // FixItHints associated with the definition being checked.
+  llvm::SmallVector<clang::FixItHint, 4> Hints;
+
+  // Locations of any declarations that could not be fixed.
+  llvm::SmallVector<clang::SourceLocation, 4> DeclLocs;
+};
+
+} // namespace
+
+// Does the actual work of the check.
+static CheckResult checkDef(const clang::FunctionDecl *Def,
+                            const MatchFinder::MatchResult &MatchResult) {
+  CheckResult Result;
+  llvm::Optional<Token> Tok = findConstToRemove(Def, MatchResult);
+  if (!Tok)
+    return Result;
+
+  Result.ConstRange =
+      CharSourceRange::getCharRange(Tok->getLocation(), Tok->getEndLoc());
+  Result.Hints.push_back(FixItHint::CreateRemoval(Result.ConstRange));
+
+  // Fix the definition and any visible declarations, but don't warn
+  // seperately for each declaration. Instead, associate all fixes with the
+  // single warning at the definition.
+  for (const FunctionDecl *Decl = Def->getPreviousDecl(); Decl != nullptr;
+       Decl = Decl->getPreviousDecl()) {
+    if (llvm::Optional<Token> T = findConstToRemove(Decl, MatchResult))
+      Result.Hints.push_back(FixItHint::CreateRemoval(
+          CharSourceRange::getCharRange(T->getLocation(), T->getEndLoc())));
+    else
+      // `getInnerLocStart` gives the start of the return type.
+      Result.DeclLocs.push_back(Decl->getInnerLocStart());
+  }
+  return Result;
+}
+
+void ConstReturnTypeCheck::registerMatchers(MatchFinder *Finder) {
+  // Find all function definitions for which the return types are `const`
+  // qualified.
+  Finder->addMatcher(
+      functionDecl(returns(isConstQualified()), isDefinition()).bind("func"),
+      this);
+}
+
+void ConstReturnTypeCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Def = Result.Nodes.getNodeAs<FunctionDecl>("func");
+  CheckResult CR = checkDef(Def, Result);
+  {
+    // Clang only supports one in-flight diagnostic at a time. So, delimit the
+    // scope of `Diagnostic` to allow further diagnostics after the scope.  We
+    // use `getInnerLocStart` to get the start of the return type.
+    DiagnosticBuilder Diagnostic =
+        diag(Def->getInnerLocStart(),
+             "return type %0 is 'const'-qualified at the top level, which may "
+             "reduce code readability without improving const correctness")
+        << Def->getReturnType();
+    if (CR.ConstRange.isValid())
+      Diagnostic << CR.ConstRange;
+    for (auto &Hint : CR.Hints)
+      Diagnostic << Hint;
+  }
+  for (auto Loc : CR.DeclLocs)
+    diag(Loc, "could not transform this declaration", DiagnosticIDs::Note);
+}
+
+} // 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 "ConstReturnTypeCheck.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<ConstReturnTypeCheck>(
+        "readability-const-return-type");
     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

 
 /// Re-lex the provide \p Range and return \c false if either a macro spans
 /// multiple tokens, a pre-processor directive or failure to retrieve the
 /// next token is found, otherwise \c true.
 bool rangeContainsExpansionsOrDirectives(SourceRange Range,
                                          const SourceManager &SM,
                                          const LangOptions &LangOpts);
 
+/// Assuming that ``Range`` spans a const-qualified type, returns the ``const``
+/// token in ``Range`` that is responsible for const qualification. ``Range``
+/// must be valid with respect to ``SM``.  Returns ``None`` if no ``const``
+/// tokens are found.
+llvm::Optional<Token> getConstQualifyingToken(CharSourceRange Range,
+                                              const 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;
 }
 
 SourceLocation findPreviousTokenStart(SourceLocation Start,
                                       const SourceManager &SM,
                                       const LangOptions &LangOpts) {
   if (Start.isInvalid() || Start.isMacroID())
     return SourceLocation();
 
   SourceLocation BeforeStart = Start.getLocWithOffset(-1);
   if (BeforeStart.isInvalid() || BeforeStart.isMacroID())
     return SourceLocation();
 
   return Lexer::GetBeginningOfToken(BeforeStart, SM, LangOpts);
 }
 
 SourceLocation findPreviousTokenKind(SourceLocation Start,
                                      const SourceManager &SM,
                                      const LangOptions &LangOpts,
while (Loc < Range.getEnd()) {
     if (Tok->is(tok::hash))
       return true;
 
     Loc = Lexer::getLocForEndOfToken(Loc, 0, SM, LangOpts).getLocWithOffset(1);
   }
 
   return false;
 }
+
+llvm::Optional<Token> getConstQualifyingToken(CharSourceRange Range,
+                                              const ASTContext &Context,
+                                              const SourceManager &SM) {
+  std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(Range.getBegin());
+  StringRef File = SM.getBufferData(LocInfo.first);
+  Lexer RawLexer(SM.getLocForStartOfFile(LocInfo.first), Context.getLangOpts(),
+                 File.begin(), File.data() + LocInfo.second, File.end());
+  llvm::Optional<Token> FirstConstTok;
+  Token LastTokInRange;
+  Token Tok;
+  while (!RawLexer.LexFromRawLexer(Tok) &&
+         Range.getEnd() != Tok.getLocation() &&
+         !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) && !FirstConstTok)
+      FirstConstTok = Tok;
+    LastTokInRange = Tok;
+  }
+  // If the last token in the range is a `const`, then it const qualifies the
+  // type.  Otherwise, the first `const` token, if any, is the qualifier.
+  return LastTokInRange.is(tok::kw_const) ? LastTokInRange : FirstConstTok;
+}
 } // 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-duration-factory-float
   <clang-tidy/checks/abseil-duration-factory-float>` check.
 
   Checks for cases where the floating-point overloads of various
   ``absl::Duration`` factory functions are called when the more-efficient
   integer versions could be used instead.
- New :doc:`modernize-deprecated-ios-base-aliases
   those that have a non-deprecated equivalent.
 
 - New :doc:`readability-isolate-decl
   <clang-tidy/checks/readability-isolate-declaration>` check.
 
   Detects local variable declarations declaring more than one variable and
   tries to refactor the code to one statement per declaration.
 
+- New :doc:`readability-const-return-type
+  <clang-tidy/checks/readability-const-return-type>` 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.
 
 - New :doc:`readability-uppercase-literal-suffix
   <clang-tidy/checks/readability-uppercase-literal-suffix>` check.

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    abseil-duration-division
    abseil-duration-factory-float
    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-return-type
    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-return-type.rst

+.. title:: clang-tidy - readability-const-return-type
+
+readability-const-return-type
+=============================
+
+Checks for functions with a ``const``-qualified return type and recommends
+removal of the ``const`` keyword. Such use of `const` is usually superfluous,
+and can prevent valuable compiler optimizations.  Does not (yet) fix trailing
+return types.
+
+Examples:
+
+.. code-block:: c++
+    
+  const int foo();
+  const Clazz foo();
+  Clazz *const foo();
+
+Note that this applies strictly to top-level qualification, which excludes
+pointers or references to const values. For example, these are fine:
+
+.. code-block:: c++
+
+   const int* foo();
+   const int& foo();
+   const Clazz* foo();

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

+// RUN: %check_clang_tidy %s readability-const-return-type %t -- -- -isystem
+
+//  p# = positive test
+//  n# = negative test
+
+#include <type_traits>
+
+const int p1() {
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness
+// CHECK-FIXES: int p1() {
+  return 1;
+}
+
+const int p15();
+// CHECK-FIXES: int p15();
+
+template <typename T>
+const int p31(T v) { return 2; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu
+// CHECK-FIXES: int p31(T v) { return 2; }
+
+// We detect const-ness even without instantiating T.
+template <typename T>
+const T p32(T t) { return t; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const T' is 'const'-qual
+// CHECK-FIXES: T p32(T t) { return t; }
+
+// However, if the return type is itself a template instantiation, Clang does
+// not consider it const-qualified without knowing `T`.
+template <typename T>
+typename std::add_const<T>::type n15(T v) { return v; }
+
+template <typename A>
+class Klazz {
+public:
+  Klazz(A) {}
+};
+
+class Clazz {
+ public:
+  Clazz *const p2() {
+    // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'Clazz *const' is 'co
+    // CHECK-FIXES: Clazz *p2() {
+    return this;
+  }
+
+  Clazz *const p3();
+  // CHECK-FIXES: Clazz *p3();
+
+  const int p4() const {
+    // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const
+    // 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: return type 'const Clazz' is 'const
+  // CHECK-FIXES: Clazz operator++(int x) {
+  }
+
+  struct Strukt {
+    int i;
+  };
+
+  const Strukt p6() {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const Clazz::Strukt' i
+  // 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();
+
+  // const-qualifier is the first `const` token, but not the first token.
+  static const int p8() {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const int' is 'const'-
+  // CHECK-FIXES: static int p8() {}
+
+  static const Strukt p9() {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: return type 'const Clazz::Strukt' i
+  // 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]]:1: warning: return type 'Clazz *const' is 'cons
+  // CHECK-FIXES: Clazz *Clazz::p3() {
+  return this;
+}
+
+const Klazz<const int>* const Clazz::p5() const {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const int> *
+// CHECK-FIXES: const Klazz<const int>* Clazz::p5() const {}
+
+const Clazz::Strukt* const Clazz::p7() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Clazz::Strukt *con
+// CHECK-FIXES: const Clazz::Strukt* Clazz::p7() {}
+
+Clazz *const p10();
+// CHECK-FIXES: Clazz *p10();
+
+Clazz *const p10() {
+  // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'Clazz *const' is 'cons
+  // CHECK-FIXES: Clazz *p10() {
+  return new Clazz();
+}
+
+const Clazz bar;
+const Clazz *const p11() {
+  // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Clazz *const' is
+  // CHECK-FIXES: const Clazz *p11() {
+  return &bar;
+}
+
+const Klazz<const int> p12() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const int>'
+// CHECK-FIXES: Klazz<const int> p12() {}
+
+const Klazz<const int>* const p13() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const int> *
+// CHECK-FIXES: const Klazz<const int>* p13() {}
+
+// re-declaration of p15.
+const int p15();
+// CHECK-FIXES: int p15();
+
+const int p15() {
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning:
+// CHECK-FIXES: int p15() {
+  return 0;
+}
+
+// Exercise the lexer.
+
+const /* comment */ /* another comment*/ int p16() { return 0; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning:
+// CHECK-FIXES: /* comment */ /* another comment*/ int p16() { return 0; }
+
+/* comment */ const
+// CHECK-MESSAGES: [[@LINE-1]]:15: warning:
+// 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 'const int' is 'const'-qu
+
+#define CONST const
+CONST int p19() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu
+
+using ty = const int;
+ty p21() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'ty' (aka 'const int') is
+
+typedef const int ty2;
+ty2 p22() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'ty2' (aka 'const int') i
+
+// 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-NOTE: [[@LINE-1]]:1: note: could not transform this declaration
+
+const int p23();
+// CHECK-FIXES: int p23();
+
+const int p23() { return 3; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu
+// CHECK-FIXES: int p23() { return 3; }
+
+int const p24() { return 3; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu
+// CHECK-FIXES: int p24() { return 3; }
+
+int const * const p25(const int* p) { return p; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int *const' is 'co
+// CHECK-FIXES: int const * p25(const int* p) { return p; }
+
+// We cannot (yet) fix instances that use trailing return types, but we can
+// warn.
+auto p26() -> const int { return 3; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu
+auto p27() -> int const { return 3; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const int' is 'const'-qu
+
+std::add_const<int>::type p28() { return 3; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'std::add_const<int>::typ
+
+// p29, p30 are based on
+// llvm/projects/test-suite/SingleSource/Benchmarks/Misc-C++-EH/spirit.cpp:
+template <class T>
+Klazz<T const> const p29(T const &t) { return {}; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const T>' is
+// CHECK-FIXES: Klazz<T const> p29(T const &t) { return {}; }
+
+Klazz<char const *> const p30(char const *s) { return s; }
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<const char *
+// CHECK-FIXES: Klazz<char const *> p30(char const *s) { return s; }
+
+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();