martong added a comment.
With [modern-type-lookup], we completely evade the use of
`ASTImporterDelegate`? That would be a wonderful thing to use only the the
ExternalASTMerger on a long term...
> ... a bunch of duplicated declarations
This popped a few thoughts into my head:
One way to discover duplicated declarations is to set the ODRViolation handling
strategy to "conservative" in clang::ASTImporter. However, minimal imported
decls will be structurally non-equivalent with those which are imported in
normal mode. Thus, it may make sense only if the normal import mode is used
exclusively.
> The next step is to run the ASTImporter for these temporary expressions with
> the MinimalImport mode disabled, but that's a follow up patch.
👍 I think this is a very good direction.
================
Comment at: clang/include/clang/AST/ExternalASTMerger.h:120
+ /// declaration. If any ASTImporter did import the given declaration,
+ /// then this functions returns the declaration that FromD was imported from.
+ /// Returns nullptr if no ASTImporter did import import FromD.
----------------
typo: `functions` -> `function`
`FromD` -> `D` ?
================
Comment at: clang/lib/AST/DeclBase.cpp:95
DeclContext *Parent, std::size_t Extra) {
+ if (!(!Parent || &Parent->getParentASTContext() == &Ctx)) {
+ llvm::errs() << Parent << " | " << &Parent->getParentASTContext()
----------------
Left over debug printout?
================
Comment at: clang/lib/AST/ExternalASTMerger.cpp:108
+ /// from.
+ llvm::DenseMap<Decl *, Decl *> ToOrigin;
+ /// @see ExternalASTMerger::ImporterSource::Merger
----------------
I was thinking about that `ToOrigin` could be in the SharedState, but then I
realized it is better to have it here, because the merger is the one which
encapsulates the handling of several importers.
================
Comment at: clang/lib/AST/ExternalASTMerger.cpp:141
+ // that doesn't cause having minimally imported declarations in the target
+ // ASTContext that no connected ASTImporter has imported (and can
complete).
+ //
----------------
This line/sentence is hard to parse for me.
I get this part:
```
This way the ExternalASTMerger can safely do a minimal import that doesn't
cause having minimally imported declarations in the target ASTContext.
```
But this I don't:
```
that no connected ASTImporter has imported.
```
================
Comment at: clang/lib/AST/ExternalASTMerger.cpp:149
+ // the other (persistent) ASTImporter to this (temporary) ASTImporter.
+ // The steps can be visualized like this:
+ //
----------------
:+1:
I like this approach.
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68326/new/
https://reviews.llvm.org/D68326
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
