[Lldb-commits] [PATCH] D68326: [lldb][modern-type-lookup] No longer import temporary declarations into the persistent AST

[Gabor Marton via Phabricator via lldb-commits]

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