Page MenuHomePhabricator

[ASTImporter] Support for importing UsingDecl and UsingShadowDecl

Authored by khazem on Nov 28 2016, 6:05 PM.



Some of this patch comes from Aleksei's branch [1], with minor revisions. I've added unit tests and AST Matcher support. Copying in Manuel in case there is no need for a dedicated usingShadowDecl() matcher, in which case I could define it only in the test suite.

This patch depends on D26753.


Diff Detail


Event Timeline

khazem updated this revision to Diff 79488.Nov 28 2016, 6:05 PM
khazem retitled this revision from to [ASTImporter] Support for importing UsingDecl and UsingShadowDecl.
khazem updated this object.
khazem added reviewers: spyffe, a.sidorin.
khazem added subscribers: cfe-commits, phosek, seanklein, klimek.

Can you add a test to ASTMatchersNodeTests.cpp? Otherwise LG for the matcher parts.

a.sidorin edited edge metadata.Nov 29 2016, 9:10 AM

Thank you Kareem, It looks mostly good, but I'd like to have some functional tests in ASTMerge for this patch.

4299 ↗(On Diff #79488)


4323 ↗(On Diff #79488)

This will assert if import fails. We need to use cast_or_null and check for null returns. (If this is the result of my misleading code, sorry for this.)

4335 ↗(On Diff #79488)


4342 ↗(On Diff #79488)

This will assert if import fails. We need to use cast_or_null and check for null returns.

spyffe edited edge metadata.Nov 29 2016, 9:46 AM

Looks good, but I have a concern about the underlying branch's apparently inconsistent initialization of DeclarationNameInfo. Aleksei, could you clarify how that code works? Should we have a helper function so we don't have to carefully repeat this pattern everywhere?

4305 ↗(On Diff #79488)

I've seen this pattern before, in D20733, at ASTImporter.cpp:6488:

DeclarationNameInfo NameInfo(E->getName(), E->getNameLoc());
ImportDeclarationNameLoc(E->getNameInfo(), NameInfo);

That code didn't do the Import during the initialization of the DeclarationNameInfo. Is either of these incorrect?

a.sidorin added inline comments.Apr 10 2017, 10:43 AM
4305 ↗(On Diff #79488)

This may come from our published code but it looks like an issue that needs to be fixed. Thank you for spotting this.

Hello Kareem,

While the functionality of this patch is already implemented in my already merged patch, I added your tests into the repo. Thank you!

This revision was automatically updated to reflect the committed changes.