[clang-tidy] modernize-use-trailing-return-type check

[bernhardmgruber]

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)

[bernhardmgruber]

rebased from release_70 onto master

[JonasToth]

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:

• https://llvm.org/docs/ProgrammersManual.html
• https://llvm.org/docs/DeveloperPolicy.html
• https://llvm.org/docs/CodingStandards.html

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.

[bernhardmgruber]

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]

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.

[JonasToth]

• 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]

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]

@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]

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]

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]

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]

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]

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]

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]

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

[bernhardmgruber]

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?

[bernhardmgruber]

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

[bernhardmgruber]

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

[bernhardmgruber]

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]

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]

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

[bernhardmgruber]

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!

[JonasToth]

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

117

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.

[bernhardmgruber]

Thank you again @JonasToth for all your valueable input! I could almost successfully run my check on the llvm/lib subfolder. I created a compilation database from within Visual Studio using an extension called SourceTrail. One of the issues was the following:

struct Object { long long value; };
class C {
  int Object;
  struct Object m();
};
Object C::m() { return {0}; }

If I rewrite this to the following

struct Object { long long value; };
class C {
  int Object;
  struct Object m();
};
auto C::m() -> Object { return {0}; }

a compilation error occurs afterwards, because Object now refers to the class member. I discovered a similar problem colliding with the name of a function argument. So essentially, I believe I now require a name lookup of the return type (if it is unqualified) in the scope of the function. In case such a name already exists, i have to prefix struct/class or perform no replacement. I looked at the documentation of ASTContext, but it seems all the good stuff is in DeclContext, which I have not available. How can I perform a name lookup inside the check member function?

Thank you for any tips! And thank you for the decltype hint, I will add some more tests.

[JonasToth]

In D56160#1367074, @bernhardmgruber wrote:

Thank you again @JonasToth for all your valueable input! I could almost successfully run my check on the llvm/lib subfolder. I created a compilation database from within Visual Studio using an extension called SourceTrail. One of the issues was the following:

Very good!

struct Object { long long value; };
class C {
  int Object;
  struct Object m();
};
Object C::m() { return {0}; }

If I rewrite this to the following

struct Object { long long value; };
class C {
  int Object;
  struct Object m();
};
auto C::m() -> Object { return {0}; }

a compilation error occurs afterwards, because Object now refers to the class member. I discovered a similar problem colliding with the name of a function argument. So essentially, I believe I now require a name lookup of the return type (if it is unqualified) in the scope of the function. In case such a name already exists, i have to prefix struct/class or perform no replacement. I looked at the documentation of ASTContext, but it seems all the good stuff is in DeclContext, which I have not available. How can I perform a name lookup inside the check member function?

That is very interesting and I was not aware of this possibility :D

Every Decl derives from DeclContext, see for example the docs for CXXMethodDecl (https://clang.llvm.org/doxygen/classclang_1_1CXXMethodDecl.html)
You should be able to look up the names you are interested. I don't know of a good way to check for the issue we have right now, @aaron.ballman knows that probably better then I do.
Try to experiment, but If you don't find a solution come back to us, we will figure something out (or ask in IRC).

[aaron.ballman]

In D56160#1367811, @JonasToth wrote:

In D56160#1367074, @bernhardmgruber wrote:

Thank you again @JonasToth for all your valueable input! I could almost successfully run my check on the llvm/lib subfolder. I created a compilation database from within Visual Studio using an extension called SourceTrail. One of the issues was the following:

Very good!

struct Object { long long value; };
class C {
  int Object;
  struct Object m();
};
Object C::m() { return {0}; }

If I rewrite this to the following

struct Object { long long value; };
class C {
  int Object;
  struct Object m();
};
auto C::m() -> Object { return {0}; }

a compilation error occurs afterwards, because Object now refers to the class member. I discovered a similar problem colliding with the name of a function argument. So essentially, I believe I now require a name lookup of the return type (if it is unqualified) in the scope of the function. In case such a name already exists, i have to prefix struct/class or perform no replacement. I looked at the documentation of ASTContext, but it seems all the good stuff is in DeclContext, which I have not available. How can I perform a name lookup inside the check member function?

That is very interesting and I was not aware of this possibility :D

Every Decl derives from DeclContext, see for example the docs for CXXMethodDecl (https://clang.llvm.org/doxygen/classclang_1_1CXXMethodDecl.html)
You should be able to look up the names you are interested. I don't know of a good way to check for the issue we have right now, @aaron.ballman knows that probably better then I do.
Try to experiment, but If you don't find a solution come back to us, we will figure something out (or ask in IRC).

I think you may be able to skip the lookup (which could get expensive) and instead rely on the fact that the user must have explicitly written that type as an elaborated type specifier when trying to calculate the source range for the return type. I suspect what's happening right now is that findReturnTypeAndCVSourceRange() isn't noticing the struct specifier and that's why it's getting dropped. If the user wrote it as an elaborated type specifier, we should probably retain that when shifting it to a trailing return type.

clang-tidy/modernize/UseTrailingReturnCheck.cpp

133

I think you may need to check here if the return type is an elaborated type (e.g., one that is written as struct S rather than S).

154

And then look for the elaborated tag type here.

[bernhardmgruber]

Thank you for the feedback!

@JonasToth I added tests for decltype and i can rewrite all signatures where decltype is not the top level expression. The reason is, that the source range of a clang::DecltypeType is not accurate, as it does not include the expression within the parenthesis following the decltype keyword. There is even a FIXME somewhere in the corresponding source file.

@aaron.ballman I am not sure what you mean and maybe you have not understood my issue correctly.

In D56160#1369986, @aaron.ballman wrote:

...

I think you may be able to skip the lookup (which could get expensive) and instead rely on the fact that the user must have explicitly written that type as an elaborated type specifier when trying to calculate the source range for the return type. I suspect what's happening right now is that findReturnTypeAndCVSourceRange() isn't noticing the struct specifier and that's why it's getting dropped. If the user wrote it as an elaborated type specifier, we should probably retain that when shifting it to a trailing return type.

Given the following source code before the rewriting:

struct Object { long long value; };
class C {
  int Object;
  struct Object m();
};
Object C::m() { return {0}; }

The member function struct Object m(); needs to have a struct before Object, because otherwise, the return type would conflict with the member variable of the same name. On the contrary, the out-of-line definition Object C::m() { return {0}; } does not need the struct, as the scope of the return type does not include the member variables of class C. However, rewriting the out-of-line definition from Object C::m() { return {0}; } to auto C::m() -> Object { return {0}; } changes the scope of the return type, which now includes the member variable int Object; and results in a compilation error.
I do not understand what you meant by

the user must have explicitly written that type as an elaborated type specifier

because in case of the out-of-line defintion, the user is not required to do so. Also, when my check rewrites struct Object m();, it correctly produces auto m() -> struct Object; (findReturnTypeAndCVSourceRange() includes the struct).

I tried to circumvent the problem doing something like (F is the matched FunctionDecl)

if (auto M = dyn_cast<CXXMethodDecl>(F)) {
  if (auto Name = M->getDeclaredReturnType().getBaseTypeIdentifier()) {
    auto result = M->getDeclContext()->lookup(Name);
    if (!result.empty()) {
      // cannot do rewrite, collision
    }
  }
}

but then I noticed, the lookup is only performed in the scope of the member function's class, not including e.g. inherited classes. So if we extend the example with

class D : public C {
    ::Object g();
};
Object D::g() {

(note that instead of struct Object we can also qualify the type with the namespace it comes from), then the DeclContext of the out-of-line definition of the member function g is empty (it least, I do not get an output when i call dumpDeclContext). So maybe the DeclContext is not the right tool for the job. How else can I query all visible names in which a given object (in this case the matched (member) function) is declared? So I can check that the name of the return type is not already taken in the scope of the trailing return type.

Here is btw. the function case:

struct Object { long long value; };
Object j1(unsigned Object) { return {Object * 2}; }

After the rewrite, the return type conflicts with the parameter name.

I appreciate any input! I will continue to explore the problem. Maybe I can get it working by inspecting a multitude of DeclContexts.

[aaron.ballman]

In D56160#1372766, @bernhardmgruber wrote:

Thank you for the feedback!

@JonasToth I added tests for decltype and i can rewrite all signatures where decltype is not the top level expression. The reason is, that the source range of a clang::DecltypeType is not accurate, as it does not include the expression within the parenthesis following the decltype keyword. There is even a FIXME somewhere in the corresponding source file.

@aaron.ballman I am not sure what you mean and maybe you have not understood my issue correctly.

In D56160#1369986, @aaron.ballman wrote:

...

I think you may be able to skip the lookup (which could get expensive) and instead rely on the fact that the user must have explicitly written that type as an elaborated type specifier when trying to calculate the source range for the return type. I suspect what's happening right now is that findReturnTypeAndCVSourceRange() isn't noticing the struct specifier and that's why it's getting dropped. If the user wrote it as an elaborated type specifier, we should probably retain that when shifting it to a trailing return type.

Given the following source code before the rewriting:

struct Object { long long value; };
class C {
  int Object;
  struct Object m();
};
Object C::m() { return {0}; }

The member function struct Object m(); needs to have a struct before Object, because otherwise, the return type would conflict with the member variable of the same name.
On the contrary, the out-of-line definition Object C::m() { return {0}; } does not need the struct, as the scope of the return type does not include the member variables of class C. However, rewriting the out-of-line definition from Object C::m() { return {0}; } to auto C::m() -> Object { return {0}; } changes the scope of the return type, which now includes the member variable int Object; and results in a compilation error.

Understood. I was saying that if the inline member declaration uses an elaborated type specifier, the out-of-line, trailing-return-type variant needs to do so as well.

I do not understand what you meant by

the user must have explicitly written that type as an elaborated type specifier

because in case of the out-of-line defintion, the user is not required to do so.

But in the *declaration*, they had to, right?

Also, when my check rewrites struct Object m();, it correctly produces auto m() -> struct Object; (findReturnTypeAndCVSourceRange() includes the struct).

Ah, that's good to know. I was taking a stab as to why it was misbehaving and it seems I stabbed wrong. :-P

I tried to circumvent the problem doing something like (F is the matched FunctionDecl)

if (auto M = dyn_cast<CXXMethodDecl>(F)) {
  if (auto Name = M->getDeclaredReturnType().getBaseTypeIdentifier()) {
    auto result = M->getDeclContext()->lookup(Name);
    if (!result.empty()) {
      // cannot do rewrite, collision
    }
  }
}

but then I noticed, the lookup is only performed in the scope of the member function's class, not including e.g. inherited classes. So if we extend the example with

class D : public C {
    ::Object g();
};
Object D::g() {

(note that instead of struct Object we can also qualify the type with the namespace it comes from), then the DeclContext of the out-of-line definition of the member function g is empty (it least, I do not get an output when i call dumpDeclContext). So maybe the DeclContext is not the right tool for the job. How else can I query all visible names in which a given object (in this case the matched (member) function) is declared? So I can check that the name of the return type is not already taken in the scope of the trailing return type.

Here is btw. the function case:

struct Object { long long value; };
Object j1(unsigned Object) { return {Object * 2}; }

After the rewrite, the return type conflicts with the parameter name.

Ah, that's a very good point. :-/

I appreciate any input! I will continue to explore the problem. Maybe I can get it working by inspecting a multitude of DeclContexts.

I suspect the lookup operations you'd need to implement may not even be feasible from within clang-tidy. Sema is what handles the lookup logic (see SemaLookup.cpp) and you do not have access to that from within clang-tidy. It may be that we document this as a best-effort and add test cases (and update documentation) to demonstrate the problematic corner cases with FIXME comments.

[bernhardmgruber]

• rebased onto current master
• implemented a basic check for name collisions of unqualified names in the return type with function arugment names using RecursiveASTVisitor
• moved retrieval of FunctionTypeLoc out of findTrailingReturnTypeSourceLocation()
• replaced F.getReturnType().hasLocalQualifiers() by custom function hasAnyNestedLocalQualifiers(), as the former does not work if the qualifiers are not on the topmost type. E.g.: const int*. This is a PointerType without qualifiers, the const qualifier is part of the nested pointee type.
• inhibiting the rewrite, if the topmost return type is a decltype expression. the source range in this case does not include the expression in parenthesis after the decltype
• inserting an additional space after auto in case there was no space between the return type and the function name. E.g.: int*f();
• extended documentation with known limitations
• added more tests

[lebedev.ri]

I just noticed, i don't like the naming choice in the check.
UseTrailingReturn what is the "trailing return"?
This is about "trailing return *type*", no?

[bernhardmgruber]

Hi guys!

It took me far too long to come up with an update. Honestly, I was quite demotivated as it turned out preventing name collisions of unqualifed names after the rewrite was more difficult than I thought. Especially because this error occurs sparsely when I test the check on bigger code bases. The current approach with the AST visitor seems to work best and maybe I can even extend it at some point to automatically qualify colliding names. But for now, I would be really glad if we could achieve a state of the check that you guys can accept and commit.

I marked two lines with comments where I still need some feedback. Apart from that, I will run the check again on LLVM and other larger code bases to see if there are any further issues.

Thank you for your help!

clang-tidy/modernize/UseTrailingReturnCheck.cpp

79

TODO: is this an issue in RecursiveASTVisitor or is this behavior intended? Everywhere else, it calls getDerived().XXX();

336

FIXME: this feels wrong when I wrote it, but it works. I tried to fiddle with the ReturnTypeCVRange to include the next charakter, but I could not get it working without writing tooling::fixit::getText myself. Any ideas?

[JonasToth]

It took me far too long to come up with an update. Honestly, I was quite demotivated as it turned out preventing name collisions of unqualifed names after the rewrite was more difficult than I thought. Especially because this error occurs sparsely when I test the check on bigger code bases. The current approach with the AST visitor seems to work best and maybe I can even extend it at some point to automatically qualify colliding names. But for now, I would be really glad if we could achieve a state of the check that you guys can accept and commit.

I totally know that feeling! If there is a wrong transformation it would always be catched on compilation, right? I am fine with a check that handles 99% of code correct and we do have other checks that have similar limitations. If you find a way to exclude the wrong cases from transformation even better.

clang-tidy/modernize/UseTrailingReturnCheck.cpp

79

I don't know, but its not a big issue, is it?

336

That only works, because StringRef points within the orignal code buffer. IMHO just adding the space always and letting clang-format do its job is better then this potential out-of-bounds read.
Maybe you could increase the ReturnTypeCVRange by one at the end, then its fine.

[bernhardmgruber]

• renamed the check to modernize-use-trailing-return-type
• fixed the out-of-bounds access to check whether there is a space after the return type

clang-tidy/modernize/UseTrailingReturnCheck.cpp

336

I now retrieve the space after the return type explicitely. Using clang-format afterwards is not always possible on the code bases I work with, so I would like to not rely on it.

[lebedev.ri]

In D56160#1412701, @bernhardmgruber wrote:

• renamed the check to modernize-use-trailing-return-type

Thanks!

[JonasToth]

From my side only the nits are left.

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

94 ↗

(On Diff #188621)

Nit: const is only used for references or pointers.

154 ↗

(On Diff #188621)

Nit: I think this and the next empty line could be ellided. They do not add a lot of value and we try to keep the code dense.

241 ↗

(On Diff #188621)

integer bug for big Tokens.size(). Please add an assert(I <= size_t(std::numeric_limits<int>::max()) && "Integer overflow detected") or the like.
Optimally gsl::narrow<>() could be used, which we do not have in LLVM.

[bernhardmgruber]

Fixed some little nits, thanks @JonasToth!

[aaron.ballman]

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

34 ↗

(On Diff #189222)

Missing full stop at the end of the sentence.

150 ↗

(On Diff #189222)

This list misses an edge case for __restrict. e.g., https://godbolt.org/z/d4WDcA

180 ↗

(On Diff #189222)

Missing full stop.

229 ↗

(On Diff #189222)

This also misses the __restrict case.

274 ↗

(On Diff #189222)

Should this also bail out if the function is main()?

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

95–97 ↗

(On Diff #189222)

This is a rather unexpected transformation, to me.

[bernhardmgruber]

• added support for __restrict
• added two dots at end of comments

[aaron.ballman]

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

180–184 ↗

(On Diff #189993)

Perhaps I'm a bit dense on a Monday morning, but why should this be a diagnostic? I am worried this will diagnose situations like (untested guesses):

#define const const
const int f(void); // Why no fixit?

#define attr __attribute__((frobble))
attr void g(void); // Why diagnosed as needing a trailing return type?

234 ↗

(On Diff #189993)

As a torture test for this, how well does it handle a declaration like:

const long static int volatile unsigned inline long foo();

Does it get the fixit to spit out:

static inline auto foo() -> const unsigned long long int;

293 ↗

(On Diff #189993)

This seems incorrect to me; if we cannot get the type source information, why do we assume it has a return type that needs to be turned into a trailing return type?

298 ↗

(On Diff #189993)

I think this can turn into an assertion that FTL is valid.

274 ↗

(On Diff #189222)

This comment was marked as done, but I don't see any changes or mention of what should happen. I suppose the more general question is: should there be a list of functions that should not have this transformation applied, like program entrypoints? Or do you want to see this check diagnose those functions as well?

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

95–97 ↗

(On Diff #189222)

This comment is marked as done, but there are no changes or explanations.

[bernhardmgruber]

• extracting specifiers from the return type if it consists of a multitoken built-in type, and preprending it to 'auto'.
• extended matcher to include friend declarations of functions
• added many tests for return types which contain specifiers

[bernhardmgruber]

Thank you for the rich feedback @aaron.ballman. I found a solution which seems to work for many of my test cases.

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

268 ↗

(On Diff #191233)

I am not sure if this covers all types where a specifier may occur "inside" a type. Can someone come up with something other than a built-in type consisting of at least two tokens?

180–184 ↗

(On Diff #189993)

Because I would also like to rewrite functions which contain macros in the return type. However, I cannot provide a fixit in all cases. Clang can give me a SourceRange without CV qualifiers which seems to work in all my tests so far. But to include CV qualifiers I have to do some manual lexing left and right of the return type SourceRange. If I encounter macros along this way, I bail out because handling these cases becomes compilated very easily.

Your second case does not give a diagnostic, as it is not matched by the check, because it returns void.

234 ↗

(On Diff #189993)

I honestly did not believe this compiled until I put it into godbolt. And no, it is not handled.
I added your test as well as a few other ones of this kind. You could also add constexpr or inside classes friend anywhere.

I will try to come up with a solution. I guess the best would be to delete the specifiers from the extracted range type string and readd them in the order of appearance before auto.

298 ↗

(On Diff #189993)

I actually found a case where this is null: int unsigned att() __attribute__((cdecl));

274 ↗

(On Diff #189222)

How strange does

auto main(int argc, const char* argv[]) -> int {
    return 0;
}

look to you?

I think the transformation is valid here. But I can understand that people feel uneasy after typing int main ... for decades. Should we also create an option here to turn it off for main? Or just not transform it? I do not mind. If I want main to start with auto I could also do this transformation manually.

274 ↗

(On Diff #189222)

I am sorry for marking it as done. I do not know how people work here exactly and how phabricator behaves. I thought the check boxes are handled for everyone individually. Also, if I add a new comment, it is checked by default.

How are you/most people going to use clang-tidy? Do you run it regularly and automatic? Do you expect it to find zero issues once you applied the check?
In other words, if you do not want to transform some functions, do you need an option to disable the check for these, so it runs clean on the full source code?

Personally, I do not need this behavior, as I run clang-tidy manually once in a while and revert transformations I do not want before checking in the changes.

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

95–97 ↗

(On Diff #189222)

It felt a bit strange initially, but as it is a valid transformation, i have included it. Do you think we should create an option to turn this off?

95–97 ↗

(On Diff #189222)

I am sorry.

What do you think about the transformation? Should there be an option to disable transforming extern "C" statements?

[aaron.ballman]

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

359–364 ↗

(On Diff #191233)

This is suspicious and smells like a bug, to me. I can think of no reason why this function would have no type location information just because there's a trailing attribute (I assume you mean RHS here based on the example in your comment above). It may not be your bug to fix, but knowing whether this works around a bug or not would be helpful. If it is a bug, the comment should say something like "FIXME: remove when bug blah is fixed".

180–184 ↗

(On Diff #189993)

Because I would also like to rewrite functions which contain macros in the return type. However, I cannot provide a fixit in all cases. Clang can give me a SourceRange without CV qualifiers which seems to work in all my tests so far. But to include CV qualifiers I have to do some manual lexing left and right of the return type SourceRange. If I encounter macros along this way, I bail out because handling these cases becomes compilated very easily.

That makes sense, but I am worried about this bailing out because of things that are not CV qualifiers but are typically macros, like attributes. It seems like there should not be a problem providing a fixit in that situation, unless I'm misunderstanding still.

234 ↗

(On Diff #189993)

I will try to come up with a solution. I guess the best would be to delete the specifiers from the extracted range type string and readd them in the order of appearance before auto.

Alternatively, if it's easier, you can refuse to add fix-its for the situation and just add a FIXME to handle this should users ever care.

274 ↗

(On Diff #189222)

I am sorry for marking it as done. I do not know how people work here exactly and how phabricator behaves. I thought the check boxes are handled for everyone individually. Also, if I add a new comment, it is checked by default.

No worries -- that new comments are automatically marked done by default is certainly a surprise to me!

How are you/most people going to use clang-tidy? Do you run it regularly and automatic? Do you expect it to find zero issues once you applied the check? In other words, if you do not want to transform some functions, do you need an option to disable the check for these, so it runs clean on the full source code?

I think it's hard to gauge how most people do anything, really. However, I think there are people who enable certain clang-tidy checks and have them run automatically as part of CI, etc. Those kind of folks may need the ability to silence the diagnostics. We could do this in a few ways (options to control methods not to diagnose, NOLINT comments, etc).

I kind of think we don't need an option for the user to list functions not to transform. They can use NOLINT to cover those situations as a one-off. The only situation where I wonder if everyone is going to want to write NOLINT is for main(). It might make sense to have an option to not check program entrypoints, but there is technically nothing stopping someone from using a trailing return type with a program entrypoint so maybe this option isn't needed at all.

How about we not add any options and if someone files a bug report, we can address it then?

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

95–97 ↗

(On Diff #189222)

I think I'm leaning towards consistently diagnosing (and attempting to fix when possible) everything and we can add options if users ask for them. I was mostly surprised because I'm used to seeing things in extern "C" blocks being C-ish, but that is personal preference more than anything technical.

[bernhardmgruber]

It took a long while to figure out how to handle certain macro cases. Here is what I came up with:

When tokenizing the source code from the beginning of the function to the function name, I now use clang's Preprocessor to lex these tokens again and expand macros on the way. I analyse the top-level macros if they just contain specifiers or qualifiers and store this information in a ClassifiedToken. When I later try to expand the return type location to include qualifiers, or when I want to remove specifiers from the return type range, I can use this information to also include/reprosition macros which I can regard as qualifiers or specifiers. This currently solves a lot of cases where macros are part of the return type.

Function style macros as part of the return type are not supported, as they are harder to lex and expand. The check currently provides no fixit in these cases.

Other changes:

• expandIfMacroId() now expands recursively because the SourceLocation might be inside nested macros
• renamed nonMacroTokensBeforeFunctionName() into classifyTokensBeforeFunctionName()
• improved some comments
• overriding registerPPCallbacks to hijack a reference to the preprocessor
• expanding macro ids in the initial return type source range gotten from the FunctionDecl

[bernhardmgruber]

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

268 ↗

(On Diff #191233)

int static&();
Running keepSpecifiers() on all return types now.

180–184 ↗

(On Diff #189993)

#define const const
const int f(void); // Why no fixit?

This can be handled now. As well as e.g.:

#define ATT __attribute__((deprecated))
ATT const int f();

234 ↗

(On Diff #189993)

Specifiers at arbitrary locations inside the return type should be supported now.

274 ↗

(On Diff #189222)

How about we not add any options and if someone files a bug report, we can address it then?

Sounds good to me!

[aaron.ballman]

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

85–87 ↗

(On Diff #198143)

You can combine these.

88–90 ↗

(On Diff #198143)

And this can become return TraverseTypeLoc(...);

95–98 ↗

(On Diff #198143)

This can also be simplified into a single return statement rather than an if, but it's less clear to me whether that's an improvement. WDYT?

203 ↗

(On Diff #198143)

This should return llvm::None

234 ↗

(On Diff #198143)

llvm::None

246 ↗

(On Diff #198143)

llvm::None

[bernhardmgruber]

Fixed small nits suggested by @aaron.ballman. Thanks!

[aaron.ballman]

Aside from a formatting issue, this LGTM, thank you!

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

93 ↗

(On Diff #198531)

80-col limit.

[bernhardmgruber]

@aaron.ballman and @JonasToth: Thank you for the patience and all the feedback! It means a great deal to me to have a patch accepted here!

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

95–98 ↗

(On Diff #198143)

Let's simplify it.

203 ↗

(On Diff #198143)

I always wondered what the point of llvm::None, std::nullopt or boost::none is. When I write return {}; it looks like i return an empty shell, exactly how I picture an empty optional in my head. That is why I prefer it this way. I will change it of course for this patch, but would you mind giving me a short reason, why llvm::None is preferable here?

[aaron.ballman]

In D56160#1495847, @bernhardmgruber wrote:

@aaron.ballman and @JonasToth: Thank you for the patience and all the feedback! It means a great deal to me to have a patch accepted here!

You're very welcome! Do you need one of us to commit this on your behalf, or do you already have commit privileges?

[aaron.ballman]

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

203 ↗

(On Diff #198143)

AFAIK, there's no functional difference between the two and it's more a matter of consistency. Multiple different types can have the notion of a null state, and this allows you to consistently specify that null state across types in an explicit way.

I suppose there might also be a very slight argument for clarity in that a user reading the code with {} could be confused into thinking that is default constructing the contained type rather than creating an empty optional object.

[bernhardmgruber]

@aaron.ballman I do not have commit privileges and I would be very thankful, if you could commit this patch for me! Thank you!

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp

203 ↗

(On Diff #198143)

Thank you for the explanation! I see the point of being more explicit.

[aaron.ballman]

In D56160#1496391, @bernhardmgruber wrote:

@aaron.ballman I do not have commit privileges and I would be very thankful, if you could commit this patch for me! Thank you!

I've commit for you in r360345, thank you for the patch!

[aaron.ballman]

In D56160#1496594, @aaron.ballman wrote:

In D56160#1496391, @bernhardmgruber wrote:

@aaron.ballman I do not have commit privileges and I would be very thankful, if you could commit this patch for me! Thank you!

I've commit for you in r360345, thank you for the patch!

I had to revert in r360348 as the patch does not pass the testbots: http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/48063/steps/test/logs/stdio

When you correct the issues, I'm happy to reapply the patch for you. If you could also fix up the link in the release notes as part of the cleanup, that would be great (it turned out there was a typo -- I've pointed it out in the review).

docs/ReleaseNotes.rst

154–155 ↗

(On Diff #198531)

These should be modernize-use-trailing-return-type instead (there's a typo).

[bernhardmgruber]

• fixed formatting
• fixed function names in tests
• added -fexceptions to test arguments
• fixed typo in release notes

[aaron.ballman]

In D56160#1497473, @bernhardmgruber wrote:

• fixed formatting
• fixed function names in tests
• added -fexceptions to test arguments
• fixed typo in release notes

Thanks! I committed in r360438.

[Eugene.Zelenko]

Please take a look on PR44206.

[bernhardmgruber]

@Eugene.Zelenko I tried to find what you refer to by PR44206, but I could not find anything :/ Can you please provide me with a link? Thank you!

[Eugene.Zelenko]

In D56160#1769107, @bernhardmgruber wrote:

@Eugene.Zelenko I tried to find what you refer to by PR44206, but I could not find anything :/ Can you please provide me with a link? Thank you!

See https://bugs.llvm.org/show_bug.cgi?id=44206.

clang-tidy/modernize/CMakeLists.txt

add_clang_library(clangTidyModernizeModule
   UseBoolLiteralsCheck.cpp
   UseDefaultMemberInitCheck.cpp
   UseEmplaceCheck.cpp
   UseEqualsDefaultCheck.cpp
   UseEqualsDeleteCheck.cpp
   UseNoexceptCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
+  UseTrailingReturnCheck.cpp
   UseTransparentFunctorsCheck.cpp
   UseUncaughtExceptionsCheck.cpp
   UseUsingCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyReadabilityModule
   clangTidyUtils
   clangTooling
   )

clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "UseBoolLiteralsCheck.h"
 #include "UseDefaultMemberInitCheck.h"
 #include "UseEmplaceCheck.h"
 #include "UseEqualsDefaultCheck.h"
 #include "UseEqualsDeleteCheck.h"
 #include "UseNoexceptCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
+#include "UseTrailingReturnCheck.h"
 #include "UseTransparentFunctorsCheck.h"
 #include "UseUncaughtExceptionsCheck.h"
 #include "UseUsingCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
CheckFactories.registerCheck<UseDefaultMemberInitCheck>(
         "modernize-use-default-member-init");
     CheckFactories.registerCheck<UseEmplaceCheck>("modernize-use-emplace");
     CheckFactories.registerCheck<UseEqualsDefaultCheck>("modernize-use-equals-default");
     CheckFactories.registerCheck<UseEqualsDeleteCheck>(
         "modernize-use-equals-delete");
     CheckFactories.registerCheck<UseNoexceptCheck>("modernize-use-noexcept");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
+    CheckFactories.registerCheck<UseTrailingReturnCheck>(
+        "modernize-use-trailing-return");
     CheckFactories.registerCheck<UseTransparentFunctorsCheck>(
         "modernize-use-transparent-functors");
     CheckFactories.registerCheck<UseUncaughtExceptionsCheck>(
         "modernize-use-uncaught-exceptions");
     CheckFactories.registerCheck<UseUsingCheck>("modernize-use-using");
   }
 
   ClangTidyOptions getModuleOptions() override {

clang-tidy/modernize/UseTrailingReturnCheck.h

+//===--- UseTrailingReturnCheck.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_MODERNIZE_USETRAILINGRETURNCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USETRAILINGRETURNCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Rewrites function signatures to use a trailing return type.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-trailing-return.html
+class UseTrailingReturnCheck : public ClangTidyCheck {
+public:
+  UseTrailingReturnCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USETRAILINGRETURNCHECK_H

clang-tidy/modernize/UseTrailingReturnCheck.cpp

+//===--- UseTrailingReturnCheck.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 "UseTrailingReturnCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+namespace {
+
+// very similar to UseOverrideCheck
+SourceLocation findTrailingReturnTypeLocation(const CharSourceRange &range,
+                                              const ASTContext &ctx,
+                                              const SourceManager &sm,
+                                              const LangOptions &langOpts) {
+  const std::pair<FileID, unsigned> loc = sm.getDecomposedLoc(range.getBegin());
+  const StringRef file = sm.getBufferData(loc.first);
+  const char *tokenBegin = file.data() + loc.second;
+  Lexer lexer(sm.getLocForStartOfFile(loc.first), langOpts, file.begin(),
+              tokenBegin, file.end());
+  Token t;
+  int nestedParens = 0;
+  SourceLocation result;
+  while (!lexer.LexFromRawLexer(t)) {
+    if (sm.isBeforeInTranslationUnit(range.getEnd(), t.getLocation()))
+      break;
+    if (result.isValid()) {
+      // we found the closing parenthesis, skip additional const and ref
+      // qualifiers
+
+      if (t.is(tok::amp) || t.is(tok::ampamp)) {
+        result = t.getEndLoc();
+        continue;
+      }
+
+      if (t.is(tok::raw_identifier)) {
+        IdentifierInfo &Info = ctx.Idents.get(
+            StringRef(sm.getCharacterData(t.getLocation()), t.getLength()));
+        t.setIdentifierInfo(&Info);
+        t.setKind(Info.getTokenID());
+      }
+
+      if (t.is(tok::kw_const)) {
+        result = t.getEndLoc();
+        continue;
+      }
+
+      return result;
+    }
+    if (t.is(tok::l_paren))
+      ++nestedParens;
+    else if (t.is(tok::r_paren)) {
+      --nestedParens;
+      if (nestedParens == 0)
+        result = t.getEndLoc(); // store location, next token might be const
+    }
+  }
+
+  if (result.isValid())
+    return result;
+
+  llvm_unreachable("Expected to find a closing paranthesis");
+}
+
+SourceRange findReturnTypeAndCVRange(const FunctionDecl &f,
+                                     const ASTContext &ctx,
+                                     const SourceManager &sm,
+                                     const LangOptions &langOpts) {
+
+  // we start with the range of the return type and expand to neighboring const
+  // and volatile
+  auto returnTypeRange = f.getReturnTypeSourceRange();
+
+  // create tokens for everything before the name of the function
+  const std::pair<FileID, unsigned> loc = sm.getDecomposedLoc(f.getBeginLoc());
+  const StringRef file = sm.getBufferData(loc.first);
+  const char *tokenBegin = file.data() + loc.second;
+  Lexer lexer(sm.getLocForStartOfFile(loc.first), langOpts, file.begin(),
+              tokenBegin, file.end());
+  Token t;
+  std::vector<Token> tokens;
+  while (!lexer.LexFromRawLexer(t)) {
+    if (sm.isBeforeInTranslationUnit(f.getLocation(), t.getLocation()))
+      break;
+    if (t.is(tok::raw_identifier)) {
+      IdentifierInfo &Info = ctx.Idents.get(
+          StringRef(sm.getCharacterData(t.getLocation()), t.getLength()));
+      t.setIdentifierInfo(&Info);
+      t.setKind(Info.getTokenID());
+    }
+    tokens.push_back(t);
+  }
+
+  // include const and volatile to the left and right of the return type
+  auto isCV = [](Token t) {
+    return t.is(tok::kw_const) || t.is(tok::kw_volatile);
+  };
+
+  bool extendedLeft = false;
+  for (int i = 0; i < tokens.size(); i++) {
+    // if we found the beginning of the return type, include const and volatile
+    // to the left
+    if (!sm.isBeforeInTranslationUnit(tokens[i].getLocation(),
+                                      returnTypeRange.getBegin()) &&
+        !extendedLeft) {
+      for (int j = i - 1; j >= 0 && isCV(tokens[j]); j--)
+        returnTypeRange.setBegin(tokens[j].getLocation());
+      extendedLeft = true;
+    }
+    // if we found the end of the return type, include const and volatile to the
+    // right
+    if (sm.isBeforeInTranslationUnit(returnTypeRange.getEnd(),
+                                     tokens[i].getLocation())) {
+      for (int j = i; j < tokens.size() && isCV(tokens[j]); j++)
+        returnTypeRange.setEnd(tokens[j].getLocation());
+      break;
+    }
+  }
+
+  return returnTypeRange;
+}
+} // namespace
+
+void UseTrailingReturnCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus11)
+    return;
+
+  Finder->addMatcher(
+      functionDecl(unless(anyOf(hasTrailingReturn(), returns(voidType()),
+                                returns(autoType()), cxxConversionDecl())))
+          .bind("f"),
+      this);
+}
+
+void UseTrailingReturnCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *f = Result.Nodes.getNodeAs<FunctionDecl>("f");
+  if (!f)
+    return;
+
+  if (const auto *m = dyn_cast<CXXMethodDecl>(f))
+    if (m->isImplicit() || m->getLocation().isMacroID() || m->isOutOfLine())
+      return;
+
+  if (f->getDeclaredReturnType()->isFunctionPointerType()) {
+    auto d = diag(
+        f->getLocation(),
+        "use a trailing return type for this function (FixIt not implemented)");
+    return;
+  }
+
+  const auto &ctx = *Result.Context;
+  const auto &sm = *Result.SourceManager;
+  const auto &opts = getLangOpts();
+
+  auto returnTypeCVRange = findReturnTypeAndCVRange(*f, ctx, sm, opts);
+  if (!returnTypeCVRange.isValid()) {
+    diag(f->getLocation(),
+         "use a trailing return type for this function (failed to determine "
+         "return type source range, could clang resolve all #includes?)",
+         DiagnosticIDs::Level::Error);
+    return;
+  }
+
+  const CharSourceRange charRange = Lexer::makeFileCharRange(
+      CharSourceRange::getTokenRange(f->getSourceRange()), sm, opts);
+  if (!charRange.isValid())
+    return;
+  const auto insertionLoc =
+      findTrailingReturnTypeLocation(charRange, ctx, sm, opts);
+
+  // extract return type as string
+  const auto returnType = [&] {
+    // using the declared return type discards user formatting and order (order
+    // of const, volatile, type, whitespace, space before & ...) return
+    // f->getDeclaredReturnType().getAsString();
+
+    const std::pair<FileID, unsigned> beg =
+        sm.getDecomposedLoc(returnTypeCVRange.getBegin());
+    const StringRef file = sm.getBufferData(beg.first);
+    const char *begPtr = file.data() + beg.second;
+
+    const std::pair<FileID, unsigned> end = sm.getDecomposedLoc(
+        Lexer::getLocForEndOfToken(returnTypeCVRange.getEnd(), 0, sm, opts));
+    assert(beg.first == end.first);
+    assert(beg.second != end.second);
+    const char *endPtr = file.data() + end.second;
+
+    return std::string(begPtr, endPtr);
+  }();
+
+  auto d =
+      diag(f->getLocation(), "use a trailing return type for this function");
+  d << FixItHint::CreateReplacement(returnTypeCVRange, "auto");
+  d << FixItHint::CreateInsertion(insertionLoc, " -> " + returnType);
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/modernize-use-trailing-return.rst

+.. title:: clang-tidy - modernize-use-trailing-return
+
+modernize-use-trailing-return
+======================
+
+
+Rewrites function signatures to use a trailing return type.

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

+// RUN: %check_clang_tidy %s modernize-use-trailing-return %t -- -- --std=c++14
+
+namespace std {
+    template <typename T>
+    class vector;
+
+    class string;
+}
+
+//
+// Samples triggering the check
+//
+
+int f();
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}auto f() -> int;{{$}}
+int f(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}auto f(int) -> int;{{$}}
+int f(int arg);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}auto f(int arg) -> int;{{$}}
+int f(int arg1, int arg2, int arg3);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}auto f(int arg1, int arg2, int arg3) -> int;{{$}}
+int f(int arg1, int arg2, int arg3, ...);
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}auto f(int arg1, int arg2, int arg3, ...) -> int;{{$}}
+template <typename T> int f(T t);
+// CHECK-MESSAGES: :[[@LINE-1]]:27: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}template <typename T> auto f(T t) -> int;{{$}}
+int a1() { return 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}auto a1() -> int { return 42; }{{$}}
+int a2() {
+    return 42;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}auto a2() -> int {{{$}}
+int a3()
+{
+    return 42;
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}auto a3() -> int{{$}}
+int b(int   arg   )   ;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}auto b(int   arg   ) -> int   ;{{$}}
+inline int d1(int arg);
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}inline auto d1(int arg) -> int;{{$}}
+inline int d2(int arg) noexcept(true);
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}inline auto d2(int arg) -> int noexcept(true);{{$}}
+inline int d3(int arg) try { } catch(...) { }
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}inline auto d3(int arg) -> int try { } catch(...) { }{{$}}
+namespace N {
+    bool e1();
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:10: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}    auto e1() -> bool;{{$}}
+inline volatile const std::vector<std::string> e2();
+// CHECK-MESSAGES: :[[@LINE-1]]:48: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}inline auto e2() -> volatile const std::vector<std::string>;{{$}}
+inline const std::vector<std::string> volatile e2();
+// CHECK-MESSAGES: :[[@LINE-1]]:48: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}inline auto e2() -> const std::vector<std::string> volatile;{{$}}
+inline std::vector<std::string> const volatile e2();
+// CHECK-MESSAGES: :[[@LINE-1]]:48: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}inline auto e2() -> std::vector<std::string> const volatile;{{$}}
+bool N::e1() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}auto N::e1() -> bool {}{{$}}
+int (*e3())(double);
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use a trailing return type for this function (FixIt not implemented) [modernize-use-trailing-return]
+// TODO: not matched by the AST matcher
+//decltype(auto) e4();
+// _HECK-MESSAGES: :[[@LINE-1]]:16: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// _HECK-FIXES: {{^}}auto e4() -> decltype(auto);{{$}}
+
+struct B {
+    double base1(int, bool b);
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}    auto base1(int, bool b) -> double;{{$}}
+
+    virtual double base2(int, bool b) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}    virtual auto base2(int, bool b) -> double {}{{$}}
+
+    virtual float base3() const = 0;
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}    virtual auto base3() const -> float = 0;{{$}}
+
+    double base4(int, bool b) &&;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}    auto base4(int, bool b) && -> double;{{$}}
+
+    double base5(int, bool b) const &&;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}    auto base5(int, bool b) const && -> double;{{$}}
+
+    double base6(int, bool b) const & = delete;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}    auto base6(int, bool b) const & -> double = delete;{{$}}
+};
+
+struct D : B {
+    virtual double f1(int, bool b) final;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}    virtual auto f1(int, bool b) -> double final;{{$}}
+
+    virtual double base2(int, bool b) override;
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}    virtual auto base2(int, bool b) -> double override;{{$}}
+
+    virtual float base3() const override final { }
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: use a trailing return type for this function [modernize-use-trailing-return]
+// CHECK-FIXES: {{^}}    virtual auto base3() const -> float override final { }{{$}}
+};
+
+//
+// Samples which do not trigger the check
+//
+
+auto ff();
+auto f() -> int;
+auto f(int) -> int;
+auto f(int arg) -> int;
+auto f(int arg1, int arg2, int arg3) -> int;
+auto f(int arg1, int arg2, int arg3, ...) -> int;
+template <typename T> auto f(T t) -> int;
+
+void c();
+void c(int arg);
+void c(int arg) { return; }
+
+struct D2 : B {
+    virtual auto f1(int, bool b) -> double final;
+    virtual auto base2(int, bool b) -> double override;
+    virtual auto base3() const -> float override final { }
+
+    operator double();
+};