Page MenuHomePhabricator

[clang-tidy] modernize-use-trailing-return check
Needs ReviewPublic

Authored by bernhardmgruber on Sun, Dec 30, 9:44 AM.

Details

Summary

The new clang-tidy pass modernize-use-trailing-return rewrites function signatures to use a trailing return type.

A fair amount of tests are included.

Does not work on return types which span locations before and after the function name (e.g. functions returning function pointers). The pass may fail if the return types are from missing headers (e.g. when clang-tidy is run without a compilation database or all needed include directories)

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bernhardmgruber marked 2 inline comments as done.

added more test cases, allowing check to run on out-of-line definitions and updated docs.

bernhardmgruber marked 2 inline comments as done.Wed, Jan 2, 4:17 PM
bernhardmgruber added inline comments.
test/clang-tidy/modernize-use-trailing-return.cpp
3

@lebedev.ri: I do not understand what kind of tests I should write for macros. Do you mean to rewrite functions inside macro definitions as well?

like rewriting

#define DEFINE_F int f();

into

#define DEFINE_F auto f() -> int;

Apart from that, I believe a lot of difficult trickery can be done with macros and I am fine without supporting those (i.e. omitting a possible rewrite, erroneus replacements should never happen). Can you come up with a particular example you would like to be rewritten?

Eugene.Zelenko added inline comments.Wed, Jan 2, 4:22 PM
docs/ReleaseNotes.rst
42

Please rebase from trunk and use alphabetical order for new checks list.

rebased from release_70 onto master

Hi Bernhard,

thanks for you patch!
You mentioned that this is your first contribution, if you didn't find these docs already they might help you with the LLVM source-code a bit:

test/clang-tidy/modernize-use-trailing-return.cpp
3

usually we leave code inside macros alone as you said because of all the potential issues macros create.
it is best, to cover some expected macro-cases and find a reasonable (sometimes configurable) policy for them.

Some things that come to my mind (macro is all upper case):

  • RETURN_TYPE foo();
  • CONST return_type foo();
  • ALWAYS_INLINE DLL_EXPORT int foo(); ( probably the same as above)
  • int foo() ANOTHER_ATTRIBUTE;
  • int FUNCTION_NAME_MACRO(foo, bar)();
  • FULL_DEFINITION_IN_MACRO(foo, bar);
  • GTest-like function/method creation in bigger macros. Sometimes full classes with members are created in macros, in principle the same as the prior case

The safest and easiest way is to bail out anytime you find a macro-id in the range you want to transform. Some things, like case 1-5 could in theory be transformable, but should we? If you decide to transform them it is necessary to not use the expanded macro-tokens but the original source text, which should be tested too.

174

you could figure out the return type of the lambda if it contains a return, otherwise it should be void.

Big thank you to @JonasToth for providing some macro test cases. I played a bit with macros and settled on the following behavior:

  • if the closing parenthesis of the function is inside a macro, no FixIt will be created (I cannot relyably lex for subsequent CV and ref qualifiers and maybe we do not want to make changes in macros)
  • if the return type is not CV qualified, i allow macros to be part of it because no lexing is required
  • if the return type is CV qualified and contains macros, I provide no FixIt

I improved findTrailingReturnTypeSourceLocation() by discovering FunctionTypeLoc::getRParenLoc(). I still require some lexing for CV and ref qualifiers though.

I tried to improve findReturnTypeAndCVSourceRange() by finding a way to get the source range directly from the AST, but it seems the AST does not store source locations for CV qualifiers of types. I read that in the docs for QualifiedTypeLoc. So it seems I cannot circumvent finding the locations of const and volatile by my own.

bernhardmgruber marked 4 inline comments as done.Sun, Jan 6, 4:34 PM
bernhardmgruber added inline comments.
test/clang-tidy/modernize-use-trailing-return.cpp
174

I am sorry, but I do not understand what you want. Lambdas have trailing return types by default (if it is not left out and deduced). Do you want to explicitely generate the deduced return type? This is not what I intended. I want to rewrite old return types on the left to be trailing.

  • if the closing parenthesis of the function is inside a macro, no FixIt will be created (I cannot relyably lex for subsequent CV and ref qualifiers and maybe we do not want to make changes in macros)

Usually macros are untouched because its impossible to transform them correctly. Someone, somewhere does evil stuff with them and is of course mad if the transformation interfers ;)

  • if the return type is not CV qualified, i allow macros to be part of it because no lexing is required
  • if the return type is CV qualified and contains macros, I provide no FixIt

    I improved findTrailingReturnTypeSourceLocation() by discovering FunctionTypeLoc::getRParenLoc(). I still require some lexing for CV and ref qualifiers though.

    I tried to improve findReturnTypeAndCVSourceRange() by finding a way to get the source range directly from the AST, but it seems the AST does not store source locations for CV qualifiers of types. I read that in the docs for QualifiedTypeLoc. So it seems I cannot circumvent finding the locations of const and volatile by my own.

Yup.

clang-tidy/modernize/UseTrailingReturnCheck.cpp
20

auto -> const char* here, thats not nice :)

How about a llvm::StringRef or https://llvm.org/doxygen/classllvm_1_1StringLiteral.html (in this case better)

34

Please add an error-msg to the assertion like assert(TSI && "Reason why this must hold");. Humanreadable for debugging.

39

Please make that comment (and all comments in general) full grammatical sentence with correct punctuation.

43

weird formatting, did clang-format do that?

46

I think you can ellide that extra message. Not emitting the fixit is clear already.

47

Indiciator to use llvm::Optional as return type instead. What do you think?

54

Values are usually not const'ed. Please change that for consistency.

68

why are pointers not relevant here?
There should be Token.oneOf() or Token.isOneOf() or similar for this usecase

89

Same with the other extra-bit of information. This will rather confuse the user of clang-tidy then give additional information. Please ellide it.

103

please ellide const here

118

message.
You can use llvm::StringRef instead of std::string (here and elsewhere) as you don't need to own the message (after removing the extra part).

129

Token has a special method to check for one of many tokenkinds, please use that instead.

162–164

auto shuold only be used if its obvious what type the variable has (e.g. auto foo = make_unique<MyClass>(), but not for auto Foo = GetSomeResult();).
This makes review and reading easier and is the guideline for LLVM. It has room for subjective reasoning, but adding the type is usually better.

171

you can assert here, as landing here means the matcher fired.

174

What about data-pointer-member types? (https://en.cppreference.com/w/cpp/language/pointer point 2))
Its an uncommon construct, but better catch it here instead of bug-reports.

176

message.

198

Maybe https://clang.llvm.org/doxygen/namespaceclang_1_1tooling_1_1fixit.html#aebb060ec807563c615d5e004b1f6467d getText is slightly cleaner here? (it does exactly your call, but shorter)

no const for StringRef

test/clang-tidy/modernize-use-trailing-return.cpp
174

I understood the check in the way, that the trailing return type would be added always.
It is ok to leave lambdas alone if that is not what you intent.

223

What about this case and variations.

template <typename T>
using Real = T;
#define PRECISION float
Real<PRECISION> foo() { return 0.; }

Given you do some lexing I want to push that implementation a bit and try to find corner-cases.

bernhardmgruber marked 31 inline comments as done.

Fixed most of the issues pointed out by @JonasToth and added a few more tests. The tests with data member pointers currently fail and I will investiage this.

clang-tidy/modernize/UseTrailingReturnCheck.cpp
34

I replaced the asserts by an error with a message. Honestly, I do not know when these might be nullptr. It just never occured during my tests.

39

I am not a native English speaker, but the only thing that might be odd about this sentence is that , bail out. Maybe a em-dash is what we need here, but this not Latex. Oh yes, and inside of a macro is also weird.

I will make it: if the function argument list ends inside a macro, it is dangerous to start lexing from here - bail out.

I will also add dots at the end of sentences, other checks seem to follow this convention (Personally, I do not like the punctuation at the end, as it does not add significant information)

43

I use the latest version of Visual Studio and it picks up .clang-format files and formats automatically when I save. I can try to run clang-format by hand.

46

I actually like having a reason why my check could not provide a FixIt. Especially because there are multiple reasons why this might happen.

47

I absolutely agree that optional is a great type. But SourceLocation has a sentinel/empty value, which can be queried using Valid()/Invalid(). SourceLocation has the optionality built in so to speak. I have also seen other checks using a default constructed source location to indicate no result. Using optional would make the function interface more clear, but then we could have the weird case of an initialized optional containing an invalid source location. I would like to leave it as it is.

54

I do not agree to have something mutable, that should not change. Especially, now that everything else around is const, I would be suspicious why the writer had not put a const here and where the variable is modified after its initialization. But if you require it for consistency, I can remove it.

68

I am lexing CV and ref qualifiers after the closing parenthesis of a function's argument list (i.e. a member function then). I do not know what you mean with pointers here. An asterisk cannot appear as a qualifier on a method AFAIK.

Thanks for the tip!

174

Those are fine, because they do not span space before and behind the function name and argument list when declaring them. But member function pointer types also needed to be excluded. Thank you for leading me to this test case!

Edit: I just found out that the return type source range given by the AST does only cover the first token of the qualifier to the pointer:

int std::string::* e4();
~~~~~~~

I have to investigate this.

198

thanks for the tip!

test/clang-tidy/modernize-use-trailing-return.cpp
223

works, the macro is inside the return type source range that clang already gives me

bernhardmgruber marked an inline comment as done.Mon, Jan 7, 6:03 PM

@JonasToth Do you really think I should drop the extra information on why I could not provide a FixIt? If truly yes, than I would like to keep them as comments.

MyDeveloperDay added inline comments.
test/clang-tidy/modernize-use-trailing-return.cpp
2

nit: is there a reason here why you say C++14 when the code checks for C++11?

bernhardmgruber marked 2 inline comments as done.Tue, Jan 8, 2:22 AM
bernhardmgruber added inline comments.
test/clang-tidy/modernize-use-trailing-return.cpp
2

Yes. The tests contain functions with deduced return types, such as auto f();. Those require C++14. The check itself is fine with C++11.

MyDeveloperDay added inline comments.Tue, Jan 8, 3:37 AM
test/clang-tidy/modernize-use-trailing-return.cpp
2

I kind of half guessed it would be something like that after I hit submit, I noticed some checks add secondary test files which test the various versions of C++, to be honest I found this useful for the checker I'm developing, especially as the checker has some slightly different behavior with C++11 to C++17, but maybe yours doesn't

to be honest i'm also fairly new here so don't know exactly the convention

examples where this is already done in other peoples checkers

modernize-deprecated-headers-cxx03.cpp
modernize-deprecated-headers-cxx11.cpp

// RUN: %check_clang_tidy %s modernize-deprecated-headers %t -- -extra-arg-before=-isystem%S/Inputs/modernize-deprecated-headers -- -std=c++03 -v

// RUN: %check_clang_tidy %s modernize-deprecated-headers %t -- -extra-arg-before=-isystem%S/Inputs/modernize-deprecated-headers -- -std=c++11 -v
MyDeveloperDay added inline comments.Tue, Jan 8, 3:59 AM
clang-tidy/modernize/UseTrailingReturnCheck.cpp
46

@bernhardmgruber I had the same comment given to me on a review recently with regard to diag message, let me try and help you with what I though was the rational... I think the premise is something like:

  1. "no FixIt provided" is implied by the fact it isn't fixed
  2. "function type source info is missing" doesn't tell the developer what they have to do to have it be fixed

sure it helps you as the checker developer but probably that isn't much use to a developer using the checker on their code and so might confuse them.

It also makes grepping for messages in a log file difficult because it means multiple messages coming from your checker have a different pattern (although there might be a common sub pattern)

For the most part where a fixit is not supplied where it should someone would create a test case which you could consume in your tests

To be honest as a general observation as a newbie myself, what I've noticed is that a lot of checker review comments are very similar,

  • 80 characters in rst files
  • clang-format
  • alphabetic order
  • Comments with proper puncuation
  • code in comments in `XXX`
  • don't overly use auto
  • don't use anonymous namespace functions use static functions
  • run it on a big C++ project
  • run it over all of LLVM
  • consistency of coding style (elide unnecessary const)
  • elide unnecessary braces/brackets/code/etc..

We really should try and write a "Writing a clang checker, and getting it though review" primer, because I really feel for these "gaints" that we ask to review all this code, they must go over the same thing and have to present the same reasons time and time again...

which is why If you don't mind I'd like to try to help give something back by filling in some of the reasoning gaps here to a fellow new starter

Eugene.Zelenko added inline comments.Tue, Jan 8, 6:14 AM
clang-tidy/modernize/UseTrailingReturnCheck.cpp
46

I would say that we should eat own dog food :-)

I'd love to see your documentation validation scripts as part of build!

We also should regularly run Clang-tidy on BuildBot. But first we must fix existing warnings and no everybody happy if such cleanups performed by outsiders.

See PR27267 for anonymous namespaces usage.

Clang-tidy has modernize-use-auto, but not check for LLVM guidelines conformance.

Braces should be checked by readability-braces-around-statements, but proper setup is needed.

Conformance to readability-else-after-return is another typical newbies problem.

bernhardmgruber marked an inline comment as done.Wed, Jan 9, 5:36 PM

I spent some time now to get member pointers as return values working and I am afraid but it seems there is a problem with the clang AST. Given the following code in my check (where F is a FunctionDecl):

const TypeSourceInfo *TSI = F.getTypeSourceInfo();
const FunctionTypeLoc FTL = TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>();
auto rl = FTL.getReturnLoc();
rl.getSourceRange().dump(SM);
rl.getLocalSourceRange().dump(SM);
rl.castAs<MemberPointerTypeLoc>().getSourceRange().dump(SM);
rl.castAs<MemberPointerTypeLoc>().getLocalSourceRange().dump(SM);
rl.castAs<MemberPointerTypeLoc>().getBeginLoc().dump(SM);
rl.castAs<MemberPointerTypeLoc>().getStarLoc().dump(SM);

with the following input:

namespace std {
    template <typename T>
    class vector;

    class string;
}

int std::vector<std::string>::* e6();
}

yields these source locations:

<...\aa.cpp:8:1, col:5>
<...\aa.cpp:8:5>
<...\aa.cpp:8:1, col:5>
<...\aa.cpp:8:5>
...\aa.cpp:8:1
...\aa.cpp:8:5

The first is the value I usually obtain via F.getReturnTypeSourceRange(). The following lines are what I saw in my debugger when I stepped into the implementation. I believe that getStarLoc() should point at the asterisk at column 31, not at std at column 5.

I did not find an easy workaround and I would like to avoid manual lexing because of the interference with macros. For now, I will just disable the check for member pointers.

bernhardmgruber marked 8 inline comments as done.
  • Removed detailed diagnostic messages why FixIts could not be generated
  • Excluded functions returning member pointers for now
  • All tests run now
clang-tidy/modernize/UseTrailingReturnCheck.cpp
46

Thank you @MyDeveloperDay for the list of tips and rational behind the diagnostic messages. I will check this list in the future before I send new patches. Maybe it is really a good idea to put this list somewhere!

test/clang-tidy/modernize-use-trailing-return.cpp
2

Thank you for the hint! I just split my tests into C++11 and C++14 versions, but then I realized that for C++14 there are only two tests, where one is even currently commented out:

auto f1();
//decltype(auto) f2();

I exclude auto as a return type in the checker and this also excludes decltype(auto). Initially, I also wanted to rewrite decltype(auto), but I have no strong opinions. Maybe this could be an option at some point. Some users might prefer something like:

auto f() -> decltype(auto);

But then we can also discuss:

auto f() -> const auto&;

I have written all this at some point, because I loved the aesthetics of having the function name at column 6 everywhere. But I believe for now it is good enough to just put concrete types to the back.

I am satisfied with the proposed feature set for now. I will try to run the check on LLVM itself in the next days as a final test. Are there anymore feature requests or changes from your sides?

rebased on current master (there was a conflict in the release notes)

Fixed a warning when building with gcc. Added -fdeclspec when running tests to let them compile on Linux as well.

Skipping the check for functions which do not have a valid location. This occurred when I run the check on the LLVM code base. It looked like the matcher matched something like a built in operator.

JonasToth added inline comments.Tue, Jan 15, 1:08 PM
clang-tidy/modernize/UseTrailingReturnCheck.cpp
34

Nit: s/we/We

40

const is omitted in LLVM for value-types, only references and pointers are annottated with it.

47

Nit: s/if/If/

49

same const argument.

58

Nit: s/skip/Skip/

88

Nit: s/we/We/, s/const/'const'/, s/volatile/'volatile'/
We aim to highlight code-constructs to emphasize we are talking about the construct and not confuse the meaing of words.

92

I think this comment does not add value? Logically it makes sense to not return an invalid range and not work further with it regardless of reason for invalid range.

Anyhow, i would prefer a non-trailing comment.

s/happens/Happens/, s/unknow/unknown/ typo.

97

Nit: s/if/If/ (stop commenting on these now :P)

101

please ellide this const, as above

119

Please improve that comment and here I would prefer a non-trailing comment, too.
Especially formulate whats with CV and macros, the meaning has to be guessed (and can be guessed wrong).

134

please use uppercase i and j names for consistency.

137

The whole function is quite complex, involved in multiple things (lexing, diagnostics, macro-stuff) and not easy to understand.
Could you please take a look at it again and try to split it up in smaller functions?
It looks a bit fragile and parts of it might deserve separate unit-tests.

Clarifying these parts of the code is definitly worth it.

163

Shouldn't you include returns(decltypeType()) as well?

177

Please add these as Limitations to the user-facing documentation. Some other checks do the same, just grep a bit for an example-

188

drop const

test/clang-tidy/modernize-use-trailing-return.cpp
270

These tests are missing the great template fun :)

Lets start with those two examples:

template <typename Container>
[[maybe_unused]] typename Container::value_type const volatile&& myFunnyFunction(Container& C) noexcept;

and

#define MAYBE_UNUSED_MACRO [[maybe_unused]]
template <typename Container>
MAYBE_UNUSED_MACRO typename Container::value_type const volatile** const myFunnyFunction(Container& C) noexcept;

Its not necessarily nice code, but I am sure something like this is somewhere in boost for example ;)

bernhardmgruber marked 19 inline comments as done.

Addressed most of the new review comments (mainly uppercasing start of comments).

Thank you @JonasToth for providing more feedback! I will add a few more tests with templates. Maybe I should even try to run the check on Boost and see what happens.

In the meantime I might need some help: I tried running the check on LLVM last weekend using the run-clang-tidy.py file. The script eventually crashed with a segmentation fault (after 1.5 days, running on CentOS 7, 30GiB RAM) with no modifications of the source tree. I ran again and exported the fixes, but again, python failed to merge the yaml files and crashed (probably because it went out of memory). After manual merging, I ran clang-apply-replacements and it took a while, but then I also had zero modifications on my LLVM working copy. clang-apply-replacements reported a few overlapping refactorings and missing files, but that's it. What am I doing wrong?

And btw, is there an easy way to get a compilation database on Windows?

Many thanks!

clang-tidy/modernize/UseTrailingReturnCheck.cpp
119

replaced by "The CV qualifiers of the return type are inside macros"

134

i considered this with respect to the style guide, but it just looked far to unfamiliar to me. done.

163

good question! i have a unit test of the form decltype(auto) f(); and it seems to be already excluded by returns(autoType()). but i could add your suggestion as well to make it more explicit that (for now) we will not rewrite functions returning decltype(auto).

test/clang-tidy/modernize-use-trailing-return.cpp
270

You remind me of Jason Turner at CppCon 2018 who said, we should pick a toy project, for which we are sure we can handle it, because complexity will manifest itself in the details. This is exactly what is happening now.

Thank you for input, I added it to my tests!

I believe there is some sort of memory leak in the run-clang-tidy or so. I had similar experience :)
Take this with a grain of salt, as I don't recall all details: run-clang-tidy.py just takes all output from clang-tidy and prints it. A lot is redundant due to the include-horror in c++. After I implemented deduplication (not in tree, but here https://reviews.llvm.org/D54141) things got better. You don't need to run it over the whole of LLVM, but can take a subset, too. Maybe lib/ or tools/clang/lib. Note that run-clang-tidy.py reads in the compilation database and matches your path as regex against it. So lib/ includes all libs (clang, llvm, ...).

In the meantime I might need some help: I tried running the check on LLVM last weekend using the run-clang-tidy.py file. The script eventually crashed with a segmentation fault (after 1.5 days, running on CentOS 7, 30GiB RAM) with no modifications of the source tree. I ran again and exported the fixes, but again, python failed to merge the yaml files and crashed (probably because it went out of memory). After manual merging, I ran clang-apply-replacements and it took a while, but then I also had zero modifications on my LLVM working copy. clang-apply-replacements reported a few overlapping refactorings and missing files, but that's it. What am I doing wrong?

Overlapping can not be resolved, but interesting it occurs. Maybe in headers? Each TU (~ .cpp-file) will emit transformations. If they are identical they are merged, but if not there is a collision which can not be resolved. Maybe there is something going on with macros, ... that make the same header included twice not equivalent or so?

Try to run it over a subset first and then think about the issues. Trying other projects is certainly a good idea as well.

And btw, is there an easy way to get a compilation database on Windows?

No windows-user, but cmake creates one with CMAKE_EXPORT_COMPILECOMMANDS=ON that you need to pass in somewhere somehow :)

clang-tidy/modernize/UseTrailingReturnCheck.cpp
163

There should be a difference between decltype(auto) and decltype(some_expression). I will check your tests and suggest something there.

test/clang-tidy/modernize-use-trailing-return.cpp
116

Is this still the case? That should be doable with the matchers, if you have a specific question i can look at it and try to help.

Here would be a good place to test the different decltypes.

decltype(auto) foo0() { return 1 + 2; } // should be the same as ...
auto foo1() { return 1 + 2; } // this, BUT ...
decltype(1 + 2) foo2() { return 1 + 2; } // should be something else

The matching for all three of them should be done differently.

270

Templates made me sad often, after i "finished" my checks and ran them over projects ;)
Sometimes it is almost impossible to fix very weird (and uncommon) things. But its better to find such cases early and work around them.