Thank you for the quick review! Can you please commit it for me as well? Thank you!

Fixed whitespace in test code.

Thank you for the time to review this!

Btw: what is the general rule for Phabricator reviews here when to tick the Done checkbox of a comment? What is the semantic of this checkbox? Is the ticked state the same for everyone?

Ping.

Removed probably stale FIXME.

Great, it seems I forgot to submit the inline comments. Sorry about that!

• Added two more tests with a macro supplied on the command line
• Rebased onto master

Is there anything I am not seeing here that you still would like me to do? I feel like you are waiting for something obvious from my side :S.

Ping.

Ping.

In D80514#2071202, @njames93 wrote:

In D80514#2071056, @bernhardmgruber wrote:

Reuploaded diff in an attempt to trigger a CI build.

It's not working, Still no big issue, when its ready to land just the checks locally

That's exactly the issue; the tests run fine on my local machine. I run check_clang_tidy.py manually, since I lack some Unix tools for the full test suite. And the CI build that failed seems to have used the first verison of my diff, which I updated in the meantime. From my point of few the diff is ready. I am just waiting for feedback, a LGTM and a nice person who can commit it for me :)

Reuploaded diff in an attempt to trigger a CI build.

Furthermore: how can I schedule another CI build with the newest version of the diff? When I go to the failed build from Harbormaster and I click restart, it rebuilds an older version of the diff. Thanks for the info!

moved C++20 tests to a new file

• split code tables in documentation
• removed unnecessary return statement

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

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

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

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

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

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

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

• 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

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

Fixed some little nits, thanks @JonasToth!

• 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

Hi guys!

• 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

Thank you for the feedback!

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:

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.

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

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.

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

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

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?

• Removed detailed diagnostic messages why FixIts could not be generated
• Excluded functions returning member pointers for now
• All tests run now

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;

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

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.

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

rebased from release_70 onto master

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

I fixed most of the stylistic issues. Regarding the missing test cases, I will add those and do the necessary code changes. Thank you very much for pointing them out to me!

updated diff to one with full context

This is my first contribution to LLVM and I may not yet know the conventions here.