This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Support new kinds of declarations (mostly Using*)
ClosedPublic

Authored by a.sidorin on May 2 2017, 9:27 AM.

Details

Summary

Support new AST nodes:

  • UnresolvedUsingType
  • EmptyDecl
  • NamespaceAliasDecl
  • UsingDecl
  • UsingShadowDecl
  • UsingDirectiveDecl
  • UnresolvedUsingValueDecl
  • UnresolvedUsingTypenameDecl

Refactor error handling in ImportTemplateArgumentLoc() method.
Add a test for inline namespaces.

Please take a look at FIXMEs I left in some places where I'm not sure that I have chosen a correct solution.
Also, I have some doubts for duplicating NamespaceAliasDecls. I found presence of such decls harmless but maybe I'm not correct here. I left NOTE comments both in the patch and in the tests for it.

Diff Detail

Event Timeline

a.sidorin created this revision.May 2 2017, 9:27 AM
xazax.hun edited edge metadata.May 9 2017, 2:51 AM

Looks good for me, I only have a few questions. Could you reupload the diff with contexts? It might make the review easier for others.

lib/AST/ASTImporter.cpp
1311

Don't we need an Importer.Imported call here?

1461

Minor nit: do we need the any in this sentence?

a.sidorin updated this revision to Diff 98441.May 10 2017, 6:43 AM
a.sidorin added a reviewer: karkhaz.

Address review comments; add the context.

xazax.hun accepted this revision.May 10 2017, 7:09 AM
xazax.hun added a reviewer: szepet.

LGTM! Adding Peter as a reviewer, as he is more experienced with the ASTImporter than me.

This revision is now accepted and ready to land.May 10 2017, 7:09 AM
szepet added inline comments.May 16 2017, 6:13 AM
lib/AST/ASTImporter.cpp
1464

Since the Import can result nullptr (which is checked 2 lines below) this should be a cast_or_null as I see.

a.sidorin updated this revision to Diff 99668.May 20 2017, 6:20 AM

Replaced cast<> with cast_or_null<>.

a.sidorin updated this revision to Diff 99669.May 20 2017, 6:22 AM

Removed accidentally duplicated comment.

lib/AST/ASTImporter.cpp
1311

It's done a level upper in ASTImporter::ImportDecl() but I think it's worth it to add an explicit call.

1464

Nice spot, thank you!

szepet edited edge metadata.May 28 2017, 3:32 PM

Some nits added, other than these it looks good to me. Thank you!
Just more one question, I can see 3 different cases how the import returns are handled:

  • if(!ToDecl) return nullptr;
  • if(!ToDecl && FromDecl) return nullptr;
  • no handling: ObjectXY::Create(...Import(FromDecl))

My question is the following: which cases require a check and which decls can be imported without checking the return value of the import function?
(Yepp, it could be asked in more general about the Importer, since things like this would be great to follow a convention. I have found some cases where it was not obivous to me which way to check. )

lib/AST/ASTImporter.cpp
2993

nit: As I see these cases typically handled in the way:

FrPattern = .;
ToPattern = ..;
if(FrPattern && !ToPattern)

Just to avoid the nested ifstmt.

3000

I dont see the problem with moving these up , collect nad investigate them in a smallset before the Create function, then adding them to the created ToUsing decl. It could be done as a follow up patch, dont want to mark it as a blocking issue.

3042

the same nit as above

3043

Does not this causes the same FIXME problem as above?

Hello Peter,
if (!ToDecl) return nullptr; is used if original node is always non-null.
if(!ToDecl && FromDecl) return nullptr; is used if original node can be null. If the imported node is null, the result of import is null as well so such import is OK.
ObjectXY::Create(...Import(FromDecl)) is often used for source locations - as I guess, invalid source location is OK usually. It can also be used if we know that node should already be imported, but usually indicates a potential error.

a.sidorin updated this revision to Diff 102679.Jun 15 2017, 9:36 AM
a.sidorin marked an inline comment as done.

Add a FIXME.

a.sidorin added inline comments.Jun 15 2017, 9:37 AM
lib/AST/ASTImporter.cpp
2993

The logic is a bit more complicated. There are 3 cases:

  1. Both FromPattern and ToPattern are nullptrs. Just continue.
  2. FromPattern is non-null and ToPattern is null. Return error (nullptr).
  3. Both FromPattern and ToPattern are nullptrs. Do the set... action.

So, it will require nested ifs or a code like:

if (FromPattern && ToPattern)
  set...
if (FromPattern && !ToPattern)
  return nullptr;
3000

There is a chicken and egg problem: both UsingShadowDecl and UsingDecl reference each other and UsingShadowDecl gets referenced UsingDecl as a ctor argument. If you have a good idea on how to resolve this dependency correctly, please point me.

dkrupp added a subscriber: dkrupp.Jul 17 2017, 4:29 AM
a.sidorin closed this revision.Nov 21 2017, 8:17 AM

Closed with rL318776 (forgot Differential Revision, sorry).