This is an archive of the discontinued LLVM Phabricator instance.

Move decl completion out of the ASTImporterDelegate and document it [NFC]
ClosedPublic

Authored by teemperor on May 2 2019, 11:47 PM.

Details

Summary

The ASTImporterDelegate is currently responsible for both recording and also completing
types. This patch moves the actual completion and recording code outside the ASTImporterDelegate
to reduce the amount of responsibilities the ASTImporterDelegate has to fulfill.

As I anyway had to touch the code when moving I also documented and refactored most of it
(e.g. no more asserts that we call the deporting start/end function always as a pair).

Note that I had to make the ASTImporterDelegate and it's related functions public now so that
I can move out the functionality in another class (that doesn't need to be in the header).

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.May 2 2019, 11:47 PM
Herald added a project: Restricted Project. · View Herald Transcript
teemperor edited the summary of this revision. (Show Details)May 2 2019, 11:48 PM

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

lldb/source/Symbol/ClangASTImporter.cpp
250 ↗(On Diff #197920)

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.

324 ↗(On Diff #197920)

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?

teemperor updated this revision to Diff 220965.Sep 20 2019, 1:28 AM
teemperor marked 4 inline comments as done.
  • Renamed class and related variables to reflect that it is completing TagDecls.
teemperor added inline comments.Sep 20 2019, 1:29 AM
lldb/source/Symbol/ClangASTImporter.cpp
250 ↗(On Diff #197920)

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.

324 ↗(On Diff #197920)

Maybe, but that could make this patch non-NFC :) I can make this as a follow-up.

martong accepted this revision.Sep 20 2019, 5:29 AM

Looks good to me, thanks!

lldb/source/Symbol/ClangASTImporter.cpp
328 ↗(On Diff #220965)

typo: "to completed" -> "to complete"

324 ↗(On Diff #197920)

Okay.

This revision is now accepted and ready to land.Sep 20 2019, 5:29 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2019, 5:52 AM