[ASTImporter] Refactor Decl creation
ClosedPublic

Authored by martong on Jun 1 2018, 6:27 AM.

Details

Summary

Generalize the creation of Decl nodes during Import. With this patch we do the
same things after and before a new AST node is created (::Create) The import
logic should be really simple, we create the node, then we mark that as
imported, then we recursively import the parts for that node and then set them
on that node. However, the AST is actually a graph, so we have to handle
circles. If we mark something as imported (MapImported()) then we return with
the corresponding To decl whenever we want to import that node again, this way
circles are handled. In order to make this algorithm work we must ensure
things, which are handled in the generic CreateDecl<> template:

  • There are no Import() calls in between any node creation (::Create)

and the MapImported() call.

  • Before actually creating an AST node (::Create), we must check if

the Node had been imported already, if yes then return with that one.
One very important case for this is connected to templates: we may
start an import both from the templated decl of a template and from
the template itself.

Now, the virtual Imported function is called in ASTImporter::Impor(Decl *),
but only once, when the Decl is imported. One point of this refactor is to
separate responsibilities. The original Imported() had 3 responsibilities:

  • notify subclasses when an import happened
  • register the decl into ImportedDecls
  • initialise the Decl (set attributes, etc)

Now all of these are in separate functions:

  • Imported
  • MapImported
  • InitializeImportedDecl

I tried to check all the clients, I executed tests for ExternalASTMerger.cpp
and some unittests for lldb.

Diff Detail

Repository
rL LLVM
martong created this revision.Jun 1 2018, 6:27 AM
martong updated this revision to Diff 152703.Mon, Jun 25, 8:57 AM
  • Rebase from master.

Hi Gabor!

I like the change but there are also some questions.

lib/AST/ASTImporter.cpp
1659 ↗(On Diff #152703)

As I see, all usage samples require having a variable AlreadyImported used only once. How about changing the signature a bit:

bool getOrCreateDecl(ToDeclTy *&ToD, FromDeclT *FromD, Args &&... args)

with optional LLVM_NODISCARD? (Naming is a subject for discussion).
This signature also allows us to omit template arguments because we pass an argument of a templated type. So, the call will look like this:

AccessSpecDecl *ToD;
if (getOrCreateDecl(ToD, D, Importer.getToContext(), D->getAccess(), DC, Loc, ColonLoc))
  return ToD;
1922 ↗(On Diff #152703)

Could you also fix indentation here?

martong added inline comments.Tue, Jun 26, 9:31 AM
lib/AST/ASTImporter.cpp
1659 ↗(On Diff #152703)

I like your idea, because indeed we could spare the repetition of the type in most cases. However, in one particular case we have to use the original version: in VisitTypedefNameDecl where we assign either a TypeAliasDecl * or a TypedefDecl * to a pointer to their common base class, TypedefNameDecl.
So, I think it is possible to add an overload with the signature (ToDeclTy *&ToD, FromDeclT *FromD, Args &&... args) and we could use that in the simple cases (which is the majority).

About the naming I was thinking about getAlreadyImportedOrCreateNewDecl, but this appears to be very long. So if you think this is way too long then I am OK with the shorter getOrCreateDecl.

Hi Gabor,

Let's agree on getImportedOrCreateDecl() :) I think it is informative enough but is still not too long.

I realized that, this patch brakes 3 lldb tests ATM:

  • TestTopLevelExprs.py. If https://reviews.llvm.org/D48722 was merged then this test would not be broken.
  • TestPersistentTypes.py If https://reviews.llvm.org/D48773 was merged then this test would not be broken.
  • TestRecursiveTypes.py. I am still working on this. The newly introduced assert fires: Assertion (Pos == ImportedDecls.end() || Pos->second == To) && "Try to import an already imported Decl"' failed.`.
martong updated this revision to Diff 154418.Fri, Jul 6, 8:50 AM
  • Rebase from master
  • Enable some disabled tests
  • Update the isUsed flag
  • Add a new config Minimal to the structural eq check, so lldb tests can pass now
  • Return with a bool value and use LLVM_NODISCARD in CreateDecl
  • Rename CreateDecl
martong updated this revision to Diff 154421.Fri, Jul 6, 8:57 AM

Fix indentation

martong marked 2 inline comments as done.Fri, Jul 6, 8:58 AM

Hi Gabor,
I like the new syntax. There are some comments inline; most of them are just stylish.

lib/AST/ASTImporter.cpp
94 ↗(On Diff #154421)

Comments should be complete sentences: start with a capital and end with a period.

114 ↗(On Diff #154421)

these?

161 ↗(On Diff #154421)

Should we move the below operations into updateAttrAndFlags() and use it instead?

1587 ↗(On Diff #154421)

(Thinking out loud) We need to introduce some method to return StructuralEquivalenceContext in ASTImporter. But this is an item for a separate patch, not this one.

1890 ↗(On Diff #154421)

This is not strictly equivalent to the source condition. If GetImportedOrCreateDecl returns false, we'll fall to the else branch, and it doesn't seem correct to me.

4274 ↗(On Diff #154421)

Can we just ignore the return value by casting it to void here and in similar cases?

lib/AST/ASTStructuralEquivalence.cpp
895 ↗(On Diff #154421)

assume

balazske added inline comments.Sun, Jul 8, 11:38 PM
lib/AST/ASTImporter.cpp
6905 ↗(On Diff #154421)

I think this comment is not needed (or with other text). There is a case when GetAlreadyImportedOrNull is called during import of a Decl that is already imported but its definition is not yet completely imported. If this call is here we have after GetAlreadyImportedOrNull a Decl with complete definition. (Name of the function is still bad: It does not only "get" but makes update too. The ImportDefinitionIfNeeded call can be moved into the decl create function?)

martong updated this revision to Diff 154582.Mon, Jul 9, 5:38 AM
martong marked 6 inline comments as done.

Address review comments

martong added inline comments.Mon, Jul 9, 5:38 AM
lib/AST/ASTImporter.cpp
161 ↗(On Diff #154421)

updateAttrAndFlags should handle only those properties of a Decl which can change during a subsequent import. Such as isUsed and isReferenced.

There are other properties which cannot change, e.g. isImplicit.
Similarly, Decl::hasAttrs() refers to the syntactic attributes of a declaration, e.g. [[noreturn]], which will not change during a subsequent import.

Perhaps the Attr syllable is confusing in the updateAttrAndFlags() function. Thus I suggest a new name: updateFlags().

1587 ↗(On Diff #154421)

Yes, I agree. Or perhaps we should have a common isStructuralMatch(Decl*, Decl*) function which is called by the other overloads of isStructuralMatch.

1890 ↗(On Diff #154421)

Thanks, this is a very good catch. Fixed it.

6905 ↗(On Diff #154421)

Yes, I agree. Changed the text of the code, because I believe that we should do the import of the definition on the call side of GetAlreadyImportedOrNull for the case where it is needed (in ImportDeclParts).

Hi Aleksei, could you pleas take an other quick look, there were only minor changes since your last comments. Thanks!

a_sidorin accepted this revision.Wed, Jul 11, 2:59 PM

Thank you Gabor!

I apologize for the delay in reviewing patches.

This revision is now accepted and ready to land.Wed, Jul 11, 2:59 PM
This revision was automatically updated to reflect the committed changes.

I apologize for the delay in reviewing patches.

There is no need to apologize. On the contrary, we (me and my colleges at Ericsson) would like to thank you for the effort you had put into reviewing these patches. This period was hard, we provided so many patches lately, because we had the time to open source a lot of our work only from May. We still have a few minor patches, but all the major patches are in the llvm tree already (If you are interested, here you can track which patches we still plan to create: https://github.com/Ericsson/clang/projects/2 ). We always received very useful comments from you, which I think greatly increased the quality of the patches. Thanks again.