This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] modernize-use-trailing-return-type support for C++20 concepts and decltype
ClosedPublic

Authored by bernhardmgruber on May 25 2020, 3:33 AM.

Details

Summary
  • fixed flags in RUN declaration in lit test
  • added tests for C++20 concepts and requires clause
  • added manual tokenization for AutoType return types which are not plain 'auto' to find source range
    • 'const auto'
    • 'Concept<int> auto'
    • 'decltype(auto)'
  • improved formatting of documentation
  • support for 'decltype(...)'
  • added test for bug 44206, which seems to be fixed
  • removed stale FIXME

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2020, 3:33 AM
bernhardmgruber edited the summary of this revision. (Show Details)
bernhardmgruber marked 3 inline comments as done.May 25 2020, 3:36 AM
bernhardmgruber added inline comments.
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
286–303

Is there an easier way to get the token previous to the function name?

clang-tools-extra/docs/clang-tidy/checks/modernize-use-trailing-return-type.rst
29

I tried 2 online rst editors and they failed to format the code blocks inside the tables. Will this work with the clang documentation?

clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type.cpp
555

How do you want to handle these tests which require C++20? I have seen other checks use a separate file for tests which require a different language version.

Eugene.Zelenko added inline comments.May 25 2020, 6:01 AM
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
398–399

Non needed. See readability-redundant-control-flow.

clang-tools-extra/docs/clang-tidy/checks/modernize-use-trailing-return-type.rst
29

In other documentation such examples are sequential. Same below.

Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.
bernhardmgruber marked 2 inline comments as done and 2 inline comments as done.
  • split code tables in documentation
  • removed unnecessary return statement
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
398–399

Thx!

clang-tools-extra/docs/clang-tidy/checks/modernize-use-trailing-return-type.rst
29

Ok! I will separate the code blocks again.

njames93 added inline comments.May 25 2020, 9:27 AM
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type.cpp
555

Yes, move them into a seperate file and specify -std=c++20 in the run line

moved C++20 tests to a new file

bernhardmgruber marked 2 inline comments as done.May 25 2020, 11:33 AM
bernhardmgruber added inline comments.
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type.cpp
555

Ok, done. Thx!

Ping.

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!

Ping.

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!

The restart build feature is only really useful when something on trunk caused the failure and will restart the build with the diff it had when the build was first started. The CI should build each time you update the diff, not sure why it isn't, but I don't think there is a special way to fix that. As the failure was caused by the test cases you could try and run ninja check-clang-tools from your build directory to ensure things are working, Alternatively if you upload another diff it should force a new build.

Reuploaded diff in an attempt to trigger a CI build.

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

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

Ping.

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.

aaron.ballman added inline comments.Jul 13 2020, 5:03 AM
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
286–303

There isn't, but if you find you're missing source location information for something, you can also consider modifying Clang to add that source location information and mark this as a dependent patch. It's not clear to me whether that would be worth the effort for this patch or not, but my preference is to avoid these little hacks whenever possible.

294

Should we verify that End is valid before doing this pointer arithmetic with a value derived from it? For instance, what if End points into the scratch buffer because it relies on a macro defined on the command line -- will the logic still work?

431

Why do we need to check that there aren't any nested local qualifiers?

bernhardmgruber marked 3 inline comments as done.
  • Added two more tests with a macro supplied on the command line
  • Rebased onto master
aaron.ballman added inline comments.Jul 21 2020, 11:04 AM
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
431

I'm still wondering about this.

clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type.cpp
0–1

The change to the language standard line makes me wonder if the fixme below it is now stale, or if the test will fail in C++20 mode.

bernhardmgruber added a comment.EditedJul 21 2020, 1:16 PM

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

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
286–303

Thank you for the hint! I considered this for the concept location in an AutoReturnType and the expression inside a decltype AutoReturnType. Unfortunately I could not wrap my head around what is going on in SemaType.cpp :/

294

That sounded like a scary scenario! Fortunately, expandIfMacroId even expands source locations inside the scratch buffer correctly into a location in a real file. I added two tests for it and they seem to pass. Thank you for the hint!

431

Because I would like the check to rewrite e.g. const auto f(); into auto f() -> const auto;. If I omit the check for nested local qualifiers, then those kind of declarations would be skipped.

bernhardmgruber marked an inline comment as done.
bernhardmgruber edited the summary of this revision. (Show Details)

Removed probably stale FIXME.

clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type.cpp
0–1

I just ran the tests again using python .\check_clang_tidy.py -std=c++20 .\checkers\modernize-use-trailing-return-type.cpp modernize-use-trailing-return-type aa -- -- -DCOMMAND_LINE_INT=int and I did not see mentioning of the use of an uninitialized variable. But I run on Windows, maybe the issue just surfaces on another OS? Is there a way to trigger the CI again?

I removed the FIXME in question in the hope the issue resolved itself.

aaron.ballman added inline comments.Aug 8 2020, 5:44 AM
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
431

Because I would like the check to rewrite e.g. const auto f(); into auto f() -> const auto;. If I omit the check for nested local qualifiers, then those kind of declarations would be skipped.

I don't think I understand why that's desirable though? What is it about the qualifier that makes it worthwhile to repeat the type like that?

clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type.cpp
0–1

But I run on Windows, maybe the issue just surfaces on another OS? Is there a way to trigger the CI again?

I also run on Windows so I can't easily test the behavior elsewhere for you. The CI will get triggered on new patch uploads, but I still don't always trust it. The bots are usually a more reliable source of CI truth but we have no way to speculatively trigger all the bots.

riccibruno added inline comments.
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type.cpp
0–1

I can run it for you with asan/msan.

riccibruno added inline comments.Aug 8 2020, 7:28 AM
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type.cpp
0–1

No warning with either asan or msan on x86-64 linux with clang 10.

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?

Thank you for the help!

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
431

Thank you for insisting on that peculiarity! The choice is stylistically motivated to align function names:

auto f() -> int;
auto g() -> std::vector<float>;
auto& h();
const auto k();
decltype(auto) l();

vs.

auto f() -> int;
auto g() -> std::vector<float>;
auto h() -> auto&;
auto k() -> const auto; 
auto l() -> decltype(auto);

But judging from your response, this might be a surprise to users. Would you prefer having an option to enable/disable this behavior?

clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type.cpp
0–1

Thank you @riccibruno for verifying that for me!

aaron.ballman accepted this revision.Aug 10 2020, 6:29 AM

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?

I don't think we have any hard-and-fast rules for it and I suspect the ticked state differs from review to review. Personally, I appreciate when the person who was asked to do some work is the one to check the Done box signalling that they think it's done (whether it's an author fixing a comment from a reviewer or a reviewer answering a question from an author, etc). This tells me "hey, someone thinks this is done, go look at it and comment if you disagree." However, I am guessing others have a different workflow.

I don't think I have any further concerns with this patch, so LGTM.

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
431

But judging from your response, this might be a surprise to users. Would you prefer having an option to enable/disable this behavior?

Maybe it will be, maybe it won't. :-D The reason I was surprised was because it feels like a formatting related choice rather than a modernization related choice. However, I've always struggled to understand the utility of this check (it's one I disable in every .clang-tidy configuration file I can), so my reasoning may be flawed. I feel like the goal of this check isn't to format code nicely, it's to modernize to using a trailing return type where that adds clarity. But I don't think auto& func() changing into auto func() -> auto& adds clarity (I think it removes clarity because the second signature is strictly more complex than the first), and similar for qualifiers. However, I think the exact same thing about int func() changing into auto func() -> int.

Given that we document this function to rewrite all functions to a trailing return type signature, I guess the behavior you've proposed here is consistent with that goal and so I'm fine with it.

This revision is now accepted and ready to land.Aug 10 2020, 6:29 AM
bernhardmgruber marked 12 inline comments as done.Aug 14 2020, 7:18 AM

Thank you for the time to review this!

Could you please also commit it for me? Thank you!

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
431

However, I've always struggled to understand the utility of this check (it's one I disable in every .clang-tidy configuration file I can)

I am sorry to hear that, but I have heard this from many other people as well. I am sometimes questioning myself whether it was a mistake to put this check into clang-tidy and annoy a lot of people. It might have been better as a standalone tool.

I feel like the goal of this check isn't to format code nicely, it's to modernize to using a trailing return type where that adds clarity.

I like that thinking! I started with trailing return types as a stylistic choice, but I soon realized that it indeed can add clarity by shifting away complicated return types to the end of a line where they bother me less.

But I don't think auto& func() changing into auto func() -> auto& adds clarity (I think it removes clarity because the second signature is strictly more complex than the first).

With regard to clarity, you are right. And I noted down now that I shall add an option to prevent some of these rewrites. Thank you for the feedback!

aaron.ballman closed this revision.Aug 15 2020, 7:42 AM

I've commit on your behalf in 345053390ac17dd4a2e759de9e0e24c2605035db, thank you for the patch!

clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
431

I am sorry to hear that, but I have heard this from many other people as well. I am sometimes questioning myself whether it was a mistake to put this check into clang-tidy and annoy a lot of people. It might have been better as a standalone tool.

FWIW, I don't question whether it was a mistake to add it. I just figure it wasn't targeted at me, and that's fine too. I can see the utility for people who want that kind of declaration style.