This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add the check of membership in decltype for the issue #58674
ClosedPublic

Authored by lime on Nov 6 2022, 11:34 PM.

Details

Summary

Originally, the code would take a lookup result as a member in the current scope
and build a member expression accordingly, if the lookup result was not an
operand of the address operator, or it was a field declaration. However, a field
declaration may come from another class, and cause the issue #58674.

Thus, this patch fixes the issue via checking where does the field declaration
comes from, and if it comes from another class, then marks it as not member in
the current scope. The parent scopes of the current scope are also checked, as
the current scope may be associated to a lambda or friend declaration.

Diff Detail

Event Timeline

lime created this revision.Nov 6 2022, 11:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2022, 11:34 PM
lime requested review of this revision.Nov 6 2022, 11:34 PM
lime added a project: Restricted Project.
lime added a project: Restricted Project.Nov 6 2022, 11:37 PM
lime updated this revision to Diff 473578.Nov 6 2022, 11:46 PM
lime retitled this revision from Fix the GitHub issue #58674 to [Clang] Fix the GitHub issue #58674.

Remove unnecessary changes in test cases.

lime set the repository for this revision to rG LLVM Github Monorepo.Nov 6 2022, 11:51 PM
lime retitled this revision from [Clang] Fix the GitHub issue #58674 to [clang] Fix the GitHub issue #58674.Nov 7 2022, 12:54 AM
aaron.ballman added a reviewer: Restricted Project.Nov 7 2022, 8:33 AM

FWIW, it'd help reviewers out if you would use a more descriptive title for the patch than just the GitHub issue number (it's easier for us to keep reviews straight when they have more concrete titles). I made a quick pass over this review and spotted some tiny nits, but I need to give it a more thorough review when I have some spare cycles.

clang/lib/Sema/SemaExpr.cpp
2715–2717

We can simplify this since we're in the area anyway.

2718–2722

Thanks for working on this.

I agree with Aaron, can you make the commit message clearer as to what is fixed,
and mention in clang/docs/ReleaseNotes that the issue is fixed.

clang/lib/Sema/SemaExpr.cpp
2705–2711
clang/test/SemaCXX/decltype.cpp
104

I think we are reproducing the code in the issue, we should try to reproduce all the example in that issue, including the cases that already work.

116

Maybe we should add tests for

decltype(TemplateFoo<T>::value_);
decltype(Foo::nested::value);
lime updated this revision to Diff 473943.Nov 8 2022, 3:41 AM
lime retitled this revision from [clang] Fix the GitHub issue #58674 to [clang] Add the check of decltype in derived templates for issue #58674.
shafik added a subscriber: shafik.Nov 10 2022, 8:37 PM
shafik added inline comments.
clang/docs/ReleaseNotes.rst
800–801

Looks like an unrelated formatting change.

lime updated this revision to Diff 474731.Nov 11 2022, 4:55 AM
lime updated this revision to Diff 474987.Nov 13 2022, 1:04 AM
lime retitled this revision from [clang] Add the check of decltype in derived templates for issue #58674 to [clang] Add the check of membership in decltype for the issue #58674.

Remove an if-statement.

I dont really get the logic here, I'm in need of a much better commit message/description here before I can review this.

lime edited the summary of this revision. (Show Details)Nov 15 2022, 7:09 AM

T

clang/lib/Sema/SemaExpr.cpp
2671

So what I was asking above: You're making a ton of changes to this section, without properly updating the comment that is trying to explain what is happening. Please update this to match, particularly the bit around the % operands.

lime updated this revision to Diff 475768.Nov 16 2022, 4:09 AM
lime updated this revision to Diff 477735.Nov 24 2022, 4:02 AM

Rebase.

lime updated this revision to Diff 477737.Nov 24 2022, 4:06 AM

Undo unrelated format changes.

lime updated this revision to Diff 479898.Dec 3 2022, 9:22 PM

Rebase.

lime updated this revision to Diff 483815.Dec 18 2022, 6:18 AM

Rebase and ping!

Rebase and ping!

I think looks good, but I want to make sure @erichkeane finds his comments resolved, and he is in vacation until January.

erichkeane accepted this revision.Jan 3 2023, 10:13 AM
This revision is now accepted and ready to land.Jan 3 2023, 10:13 AM
lime updated this revision to Diff 486275.Jan 4 2023, 7:08 AM

Rebased. Could you please help to commit this patch? My signature is Liming Liu <gangliugangliu.ml@outlook.com>.

This revision was landed with ongoing or failed builds.Jan 4 2023, 7:47 AM
This revision was automatically updated to reflect the committed changes.
lime added a comment.Jan 5 2023, 8:36 AM

Is there a place to see the log of the powerpc64le self-built buildbot? @erichkeane

Is there a place to see the log of the powerpc64le self-built buildbot? @erichkeane

It was sent via email to us: https://lab.llvm.org/buildbot#builders/121/builds/26738