Page MenuHomePhabricator

Add support for remembering origins to ExternalASTMerger
ClosedPublic

Authored by spyffe on Sep 22 2017, 4:45 PM.

Details

Summary

At @bruno 's request, recreating this with cfe-commits as a subscriber. The original is https://reviews.llvm.org/D36589

ExternalASTMerger has hitherto relied on being able to look up any Decl through its named DeclContext chain. This works for many cases, but causes problems for function-local structs, which cannot be looked up in their containing FunctionDecl. An example case is

void f() {
  { struct S { int a; }; }
  { struct S { bool b; }; }
}

It is not possible to lookup either of the two Ses individually (or even to provide enough information to disambiguate) after parsing is over; and there is typically no need to, since they are invisible to the outside world.

However, ExternalASTMerger needs to be able to complete either S on demand. This led to an XFAIL on test/Import/local-struct, which this patch removes. The way the patch works is:

  • It defines a new data structure, ExternalASTMerger::OriginMap, which clients are expected to maintain (default-constructing if the origin does not have an ExternalASTMerger servicing it)
  • As DeclContexts are imported, if they cannot be looked up by name they are placed in the OriginMap. This allows ExternalASTMerger to complete them later if necessary.
  • As DeclContexts are imported from an origin that already has its own OriginMap, the origins are forwarded – but only for those DeclContexts that are actually used. This keeps the amount of stored data minimal.

Diff Detail

Repository
rL LLVM

Event Timeline

spyffe created this revision.Sep 22 2017, 4:45 PM
bruno added inline comments.Sep 22 2017, 5:04 PM
lib/AST/ExternalASTMerger.cpp
79

No need for else here.

171

No need for curly braces here.

187

No need for curly braces here.

272

No need for curly braces here.

285

You can probably simplify this to something like:

bool RecordOrigin = !FoundFromDC || !IsSameDC(FoundFromDC.get(), Origin.DC);

if (RecordOrigin)
  RecordOriginImpl(ToDC, Origin, Importer);
  
if (LoggingEnabled()) {
  logs() << "(ExternalASTMerger*)" << (void*)this
         << " decided";
         
  if (!RecordOrigin)
    logs() << " NOT";
    
  logs() << " to record origin (DeclContext*)" << (void*)Origin.DC
         << ", (ASTContext*)" << (void*)&Origin.AST
         << "\n";
}
tools/clang-import-test/clang-import-test.cpp
328

No need for curly braces here

spyffe updated this revision to Diff 116680.Sep 26 2017, 9:56 AM

Updates and small refactors requested by Bruno.

spyffe marked 5 inline comments as done.Sep 26 2017, 9:57 AM
bruno accepted this revision.Sep 27 2017, 10:16 AM

LGTM

This revision is now accepted and ready to land.Sep 27 2017, 10:16 AM
This revision was automatically updated to reflect the committed changes.