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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.


================
Comment at: lldb/source/Symbol/ClangASTImporter.cpp:250
+/// imported while completing the original Decls).
+class DeportQueueScope : public ClangASTImporter::NewDeclListener {
+  ClangASTImporter::ImporterDelegateSP m_delegate;
----------------
martong wrote:
> 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.
Renamed to point out that it's essentially just completing TagDecls (I didn't want to specify the members, that seems a bit vague).

And I wish I could document why we need to do this in two steps, but I didn't write that code so that's also a mystery to me :) When I'll figure this out I'll make a patch.


================
Comment at: lldb/source/Symbol/ClangASTImporter.cpp:324
+
+    NamedDecl *to_named_decl = dyn_cast<NamedDecl>(to);
+    // Check if we already deported this type.
----------------
martong wrote:
> 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?
Maybe, but that could make this patch non-NFC :) I can make this as a follow-up.


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

https://reviews.llvm.org/D61478