This is an archive of the discontinued LLVM Phabricator instance.

Added early exit for defaulted FunctionDecls.
Needs ReviewPublic

Authored by Febbe on Feb 2 2022, 1:56 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

This prevents matching of defaulted comparison operators.
fixes llvm#53355

Diff Detail

Event Timeline

Febbe created this revision.Feb 2 2022, 1:56 PM
Febbe requested review of this revision.Feb 2 2022, 1:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2022, 1:56 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Febbe added a comment.Feb 7 2022, 8:28 AM

Push, would be nice to see this in the llvm-14 release. I can also do a review for someone else as a favor.

Mostly LG with few nits. I think it might be able to make it into the 14 release if we cherry-pick into the release branch.

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

nit: probably explains the situation better

411

This scope seems unnecessary.

Febbe updated this revision to Diff 407115.Feb 9 2022, 3:58 AM
Febbe retitled this revision from Added early exit for defaulted FunctionDecls. This prevents matching of defaulted comparison operators. fixes llvm#53355 to Added early exit for defaulted FunctionDecls..
Febbe edited the summary of this revision. (Show Details)

Applied feedback

kbobyrev added inline comments.Feb 9 2022, 4:01 AM
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
407

nit: Capitalization (start with T), dot at the end.

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

nit: // looks unrelated

Febbe added inline comments.Feb 9 2022, 4:18 AM
clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp
71

I used the // to break the line there (avoids // clang-format off/on)

in my opinion, it should look like this:

template <typename T> requires(T{0}) \n

kbobyrev accepted this revision.Feb 9 2022, 4:21 AM

LG, thanks!

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

I think it's better spelled out explicitly (otherwise the intent is unclear) or just have the clang-format apply the formatting (since it doesn't seem very important here as it's a test).

This revision is now accepted and ready to land.Feb 9 2022, 4:21 AM
Febbe added a comment.Feb 9 2022, 4:27 AM

Can I still add a diff, or does this cause a revoke (apply the rest of the feedback)?
Also, is the commit added automatically to the repo, or do I / another one have to rebase it.

Sorry for those questions, this is my first contribution to llvm.

Can I still add a diff, or does this cause a revoke (apply the rest of the feedback)?
Also, is the commit added automatically to the repo, or do I / another one have to rebase it.

Sorry for those questions, this is my first contribution to llvm.

Yes, you can add changes after LGTM, it might add the warning "landed changes with unreviewed diff" but it's generally OK if the changes are the suggestions.

Someone needs to commit the patch to the monorepo. Do you have the commit access? If not, I can do that for you once you indicate you are done with the changes.

Febbe updated this revision to Diff 407200.Feb 9 2022, 10:12 AM
Febbe marked 4 inline comments as done.

Last batch of suggested changes

Febbe marked an inline comment as done.Feb 9 2022, 10:14 AM

Thank you for the review. I am done and you can commit the patch :) . I don't have the rights to commit.

This revision now requires review to proceed.May 19 2022, 5:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 5:53 AM