This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ymandel on Oct 9 2018, 10:03 AM.

Details

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.

Event Timeline

ymandel created this revision.Oct 9 2018, 10:03 AM
ymandel edited the summary of this revision. (Show Details)Oct 9 2018, 10:04 AM

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

Please enclose const in ``.

Eugene.Zelenko added a project: Restricted Project.
ymandel updated this revision to Diff 168856.Oct 9 2018, 12:07 PM

Minor changes addressing comments.

ymandel updated this revision to Diff 168857.Oct 9 2018, 12:11 PM
ymandel marked 2 inline comments as done.

Dropped unneeded clang qualifier.

ymandel updated this revision to Diff 168858.Oct 9 2018, 12:13 PM

Add missing const.

ymandel marked 3 inline comments as done.Oct 9 2018, 12:21 PM

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

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
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
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 updated this revision to Diff 169277.Oct 11 2018, 12:50 PM

Adusted code in respone to comments.

Major differences are:

  • more warning in situations where a FixIt can't be suggested.
  • more tests.
ymandel updated this revision to Diff 169280.Oct 11 2018, 12:55 PM

comment tweak

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

Please remove the double space

ymandel updated this revision to Diff 169282.Oct 11 2018, 12:58 PM

Spacing fix.

ymandel updated this revision to Diff 169290.Oct 11 2018, 1:13 PM
ymandel marked 11 inline comments as done.

Comment fix.

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

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

MTC added a subscriber: MTC.Oct 11 2018, 7:10 PM
JonasToth added inline comments.Oct 12 2018, 6:58 AM
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 :)

58

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

61

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

70

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.

84

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.

94

Twice *PrevLoc?

96

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
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
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 updated this revision to Diff 169461.Oct 12 2018, 10:58 AM
ymandel marked 3 inline comments as done.

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.

94

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

96

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
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 marked 5 inline comments as done.Oct 12 2018, 11:04 AM
ymandel added inline comments.
clang-tidy/readability/ConstValueReturnCheck.cpp
61

Thanks, this is a significant improvement.

ymandel marked an inline comment as done.Oct 12 2018, 11:05 AM
JonasToth added inline comments.Oct 12 2018, 11:50 AM
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 :)

94

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 added inline comments.Oct 12 2018, 3:00 PM
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 added inline comments.Oct 13 2018, 2:03 AM
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 added inline comments.Oct 15 2018, 9:14 AM
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 updated this revision to Diff 169938.Oct 16 2018, 6:25 PM
ymandel marked 5 inline comments as done.

Refactors generation of clang-tidy fixits and notes.

ymandel updated this revision to Diff 169941.Oct 16 2018, 6:49 PM

Changed check result into struct (was pair).

ymandel added inline comments.Oct 16 2018, 6:50 PM
clang-tidy/readability/ConstValueReturnCheck.cpp
94

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.

ymandel edited the summary of this revision. (Show Details)Oct 16 2018, 6:51 PM

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.

ymandel marked 21 inline comments as done.Oct 17 2018, 6:59 AM
ymandel marked an inline comment as done.
ymandel marked 8 inline comments as done.Oct 17 2018, 7:02 AM
aaron.ballman added inline comments.Oct 17 2018, 4:21 PM
test/clang-tidy/readability-const-value-return.cpp
174

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 updated this revision to Diff 170418.Oct 22 2018, 8:01 AM

Expanded test cases.

ymandel marked an inline comment as done.Oct 22 2018, 8:04 AM
ymandel added inline comments.
test/clang-tidy/readability-const-value-return.cpp
174

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 updated this revision to Diff 170423.Oct 22 2018, 8:13 AM
ymandel marked an inline comment as done.

Fix some message comments in tests.

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.

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.

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 updated this revision to Diff 171103.Oct 25 2018, 8:03 AM

Fix bug in const-token finding and add tests.

ymandel updated this revision to Diff 171105.Oct 25 2018, 8:09 AM

Formatting & comment tweaks.

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.

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.

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!

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

lebedev.ri added inline comments.
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 added inline comments.Oct 26 2018, 6:04 AM
clang-tidy/readability/ConstValueReturnCheck.cpp
55

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

aaron.ballman added inline comments.Oct 26 2018, 6:12 AM
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 updated this revision to Diff 171293.Oct 26 2018, 6:31 AM
ymandel marked 4 inline comments as done.

Reword comment on the check, for clarity.

ymandel added inline comments.Oct 26 2018, 6:31 AM
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 added inline comments.Oct 26 2018, 7:30 AM
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 marked 2 inline comments as done.Oct 26 2018, 10:34 AM
ymandel added inline comments.
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 added inline comments.Oct 26 2018, 12:09 PM
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 marked 2 inline comments as done.Oct 30 2018, 6:40 AM
ymandel added inline comments.
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 added inline comments.Oct 30 2018, 6:50 AM
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 updated this revision to Diff 171700.Oct 30 2018, 7:55 AM

Reworded diagnostic message and corresponding documentation.

ymandel marked 3 inline comments as done.Oct 30 2018, 8:01 AM
ymandel added inline comments.
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 added inline comments.Oct 30 2018, 8:49 AM
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 updated this revision to Diff 171764.Oct 30 2018, 12:31 PM
ymandel marked an inline comment as done.

Modify dialog printing to highlight const token.

ymandel updated this revision to Diff 171765.Oct 30 2018, 12:37 PM
ymandel marked 2 inline comments as done.

Add comment to field.

ymandel added inline comments.Oct 30 2018, 12:38 PM
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 added inline comments.Oct 30 2018, 1:45 PM
clang-tidy/readability/ConstValueReturnCheck.cpp
108

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

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.

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 updated this revision to Diff 171897.Oct 31 2018, 5:44 AM
ymandel marked 2 inline comments as done.

Small tweaks in response to comments.

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.

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.
  1. it's a whitespace issue -- the token includes all following whitespace. False. It only includes one character of whitespace.
  1. 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 updated this revision to Diff 171900.Oct 31 2018, 5:58 AM

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

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 added inline comments.Oct 31 2018, 6:46 AM
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 accepted this revision.Oct 31 2018, 7:23 AM

This LGTM, thank you for working on this check!

This revision is now accepted and ready to land.Oct 31 2018, 7:23 AM
ymandel marked 4 inline comments as done.Oct 31 2018, 10:02 AM

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.

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, 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!

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 updated this revision to Diff 171985.Oct 31 2018, 11:51 AM
ymandel marked an inline comment as done.

Rebased patch onto trunk.

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 added inline comments.Oct 31 2018, 12:13 PM
test/clang-tidy/readability-const-return-type.cpp
6 ↗(On Diff #171985)

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

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.

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

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.