[Lldb-commits] [PATCH] D61478: Move decl completion out of the ASTImporterDelegate and document it [NFC]

[Gabor Marton via Phabricator via lldb-commits]

martong added a comment.

This is a good patch, it is good to separate responsibilities and it makes cleaner how the clang::ASTImporter is used.



================
Comment at: lldb/source/Symbol/ClangASTImporter.cpp:250
+/// imported while completing the original Decls).
+class DeportQueueScope : public ClangASTImporter::NewDeclListener {
+  ClangASTImporter::ImporterDelegateSP m_delegate;
----------------
The verb `deport` is pretty vague in this context for me. Actually, what this class does is importing missing members and methods of classes. Perhaps a better name could be `ImportMembersQueueScope` ?
I still don't understand why is it needed to import the members in two steps in LLDB: 1. import the class itself without members 2. import the members. So perhaps we could have some documentation about that too here.


================
Comment at: lldb/source/Symbol/ClangASTImporter.cpp:324
+
+    NamedDecl *to_named_decl = dyn_cast<NamedDecl>(to);
+    // Check if we already deported this type.
----------------
Would it make sense to filter out those TagDecls which are already completed?
E.g.:
```
if (TagDecl *to_tag_decl = dyn_cast<TagDecl>(to))
  if (to_tag_decl->isCompleteDefinition()) // skip tags which are already completed
    return;
```
Or this would not work because there are cases when the tag is completed, but the members are still missing? If that is the case could you please document that here too?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D61478