This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Structural eq: handle DependentScopeDeclRefExpr
ClosedPublic

Authored by martong on May 23 2019, 9:12 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.May 23 2019, 9:12 AM

Hello Gabor,
I have a few questions inline.

clang/lib/AST/ASTStructuralEquivalence.cpp
124 ↗(On Diff #200996)

Should we check the equivalence of getCXXNameType() in such cases?

127 ↗(On Diff #200996)

Should we check the equivalence of the whole Name1.getCXXDeductionGuideTemplate() (with the template arguments)?

147 ↗(On Diff #200996)

llvm_unreachable()?

163 ↗(On Diff #200996)

Should we compare TemplateArgs (getTemplateArgs) somehow?

martong marked 8 inline comments as done.Jun 28 2019, 7:30 AM
martong added inline comments.
clang/lib/AST/ASTStructuralEquivalence.cpp
124 ↗(On Diff #200996)

Good catch, thanks!

127 ↗(On Diff #200996)

Good catch, thanks!

147 ↗(On Diff #200996)

Good catch, thanks!

163 ↗(On Diff #200996)

No, that is not needed. Because getQualifier() returns with a NestedNameSpecifier and then in the appropriare overload of IsStructurallyEquivalent we will investigate further the type together with the template params:

213   case NestedNameSpecifier::TypeSpec:
214   case NestedNameSpecifier::TypeSpecWithTemplate:
215     return IsStructurallyEquivalent(Context, QualType(NNS1->getAsType(), 0),
216                                     QualType(NNS2->getAsType(), 0));
martong updated this revision to Diff 207063.Jun 28 2019, 7:30 AM
martong marked 4 inline comments as done.
  • Add further checks for the DeclarationName overload

Alexei, thank you very much for the review, you caught quite a few missing things!

a_sidorin accepted this revision.Jun 30 2019, 10:21 AM

LGTM, thanks for the fixes!

This revision is now accepted and ready to land.Jun 30 2019, 10:21 AM
martong updated this revision to Diff 207354.Jul 1 2019, 9:27 AM
  • Handle ImplicitCastExpr. In Clang7 in the last two test cases we did not have any ImplicitCastExpr in the AST. With never Clang we have, so we must handle the cast expr too.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2019, 12:36 AM
a_sidorin added inline comments.Jul 2 2019, 3:41 PM
cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
173

Hi Gabor,
Is there any test for this branch?

martong marked 2 inline comments as done.Jul 3 2019, 7:39 AM
martong added inline comments.
cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
173

Yes, there are implicit tests.
I had to add this branch because after rebasing to master a new AST node is created during the parsing of the test code.
This test code

template <typename T, typename enable_if<S1<T>::value>::type>

had this AST before the rebase:

TemplateSpecializationType 0xbd2b80 'enable_if<S1<T>::value>' dependent enable_if
`-TemplateArgument expr
  `-DependentScopeDeclRefExpr 0xbd2b28 '<dependent type>' lvalue

and this after rebase:

TemplateSpecializationType 0xc0f0c0 'enable_if<S1<T>::value>' dependent enable_if
`-TemplateArgument expr
  `-ImplicitCastExpr 0xc0f090 '_Bool' <Dependent>
    `-DependentScopeDeclRefExpr 0xc0f058 '<dependent type>' lvalue

So, SameStructsInDependentScopeDeclRefExpr, DifferentStructsInDependentScopeDeclRefExpr and DifferentValueInDependentScopeDeclRefExpr do implicitly test this branch. These tests would not work without the branch for ImplicitCastExpr.