martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

Thanks for the thorough explanation about the different ASTContexts in LLDB.
The hack is indeed hideous, but seems good to me... for now until the real 
solution is born.

> So we go to D-AST to get the std::pair members. The ASTImporter is asked to 
> copy over std::pair members
>  and first checks if std::pair is already in P-AST. However, it only finds 
> the std::pair we got from E-AST, so it
>  can't use it's map of already imported declarations and does a comparison 
> between the std::pair decls we have
>  Because the ASTImporter thinks they are different declarations, it creates a 
> second std::pair

Note that LLDB uses the lenient ODR violation handling strategy 
(`ASTImporter::ODRHandlingType::Liberal`).
With the other, stricter ODR violation handling strategy, when the to be 
imported std::pair turns out to be nonequivalent with the existing one we would 
get an error instead of a new decl.
Would it make sense to change the odr handling strategy before the formatter 
turns to the ExternalASTSource?
It would not solve this particular issue, but perhaps could make discovering 
bugs like this easier.

> My preferred solution would be to complete all declarations in E-AST before 
> they get moved to P-AST

That sounds reasonable to me.

> When doing expr m we do a minimal import of std::map from D-AST to E-AST just 
> do the type checking/codegen.

There are some cases, when codegen do need the completed tagdecl, not just the 
skeleton we get via minimal import. So, there must be cases when in the E-AST 
we have a full, completed type, right? What would happen if we imported 
everything as a full declaration in the first place, instead of  using the 
minimal import? Could that work?
Perhaps it is even more intrusive then your preferred solution, but could 
greatly reduce the import related code and complexity in LLDB.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67803/new/

https://reviews.llvm.org/D67803



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to