Page MenuHomePhabricator

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

Authored by bernhardmgruber on Dec 30 2018, 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 added inline comments.Jan 7 2019, 6:01 PM
clang-tidy/modernize/UseTrailingReturnCheck.cpp
197 ↗(On Diff #180419)

thanks for the tip!

bernhardmgruber marked an inline comment as done.Jan 7 2019, 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
1 ↗(On Diff #180596)

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.Jan 8 2019, 2:22 AM
bernhardmgruber added inline comments.
test/clang-tidy/modernize-use-trailing-return.cpp
1 ↗(On Diff #180596)

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.Jan 8 2019, 3:37 AM
test/clang-tidy/modernize-use-trailing-return.cpp
1 ↗(On Diff #180596)

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.Jan 8 2019, 3:59 AM
clang-tidy/modernize/UseTrailingReturnCheck.cpp
45 ↗(On Diff #180419)

@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.Jan 8 2019, 6:14 AM
clang-tidy/modernize/UseTrailingReturnCheck.cpp
45 ↗(On Diff #180419)

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.Jan 9 2019, 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
45 ↗(On Diff #180419)

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
1 ↗(On Diff #180596)

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.Jan 15 2019, 1:08 PM
clang-tidy/modernize/UseTrailingReturnCheck.cpp
33 ↗(On Diff #181276)

Nit: s/we/We

39 ↗(On Diff #181276)

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

46 ↗(On Diff #181276)

Nit: s/if/If/

48 ↗(On Diff #181276)

same const argument.

57 ↗(On Diff #181276)

Nit: s/skip/Skip/

87 ↗(On Diff #181276)

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.

91 ↗(On Diff #181276)

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.

96 ↗(On Diff #181276)

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

100 ↗(On Diff #181276)

please ellide this const, as above

118 ↗(On Diff #181276)

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

133 ↗(On Diff #181276)

please use uppercase i and j names for consistency.

136 ↗(On Diff #181276)

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.

162 ↗(On Diff #181276)

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

176 ↗(On Diff #181276)

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

187 ↗(On Diff #181276)

drop const

test/clang-tidy/modernize-use-trailing-return.cpp
269 ↗(On Diff #181276)

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
118 ↗(On Diff #181276)

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

133 ↗(On Diff #181276)

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

162 ↗(On Diff #181276)

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
269 ↗(On Diff #181276)

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
162 ↗(On Diff #181276)

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 ↗(On Diff #182385)

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.

269 ↗(On Diff #181276)

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 added a comment.EditedJan 22 2019, 4:38 PM

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.

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

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
132 ↗(On Diff #182385)

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

153 ↗(On Diff #182385)

And then look for the elaborated tag type here.

bernhardmgruber marked 2 inline comments as done.Jan 26 2019, 5:41 PM

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.

...

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.

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.

...

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 marked 2 inline comments as done.
  • 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

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 marked 2 inline comments as done.Wed, Feb 27, 7:00 AM

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
78 ↗(On Diff #188528)

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

335 ↗(On Diff #188528)

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?

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
78 ↗(On Diff #188528)

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

335 ↗(On Diff #188528)

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 marked 4 inline comments as done.
bernhardmgruber retitled this revision from [clang-tidy] modernize-use-trailing-return check to [clang-tidy] modernize-use-trailing-return-type check.
  • 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
335 ↗(On Diff #188528)

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.

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

Thanks!

From my side only the nits are left.

clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
95

Nit: const is only used for references or pointers.

155

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.

242

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 marked 3 inline comments as done.

Fixed some little nits, thanks @JonasToth!

aaron.ballman added inline comments.Wed, Mar 6, 9:28 AM
clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
35

Missing full stop at the end of the sentence.

151

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

181

Missing full stop.

230

This also misses the __restrict case.

275

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

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

This is a rather unexpected transformation, to me.

bernhardmgruber marked 8 inline comments as done.
  • added support for __restrict
  • added two dots at end of comments
aaron.ballman added inline comments.Mon, Mar 11, 6:11 AM
clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
180–184

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

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

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?

293

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

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

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

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