This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Remove import of definition from GetAlreadyImportedOrNull
ClosedPublic

Authored by martong on Oct 26 2018, 5:35 AM.

Diff Detail

Event Timeline

martong created this revision.Oct 26 2018, 5:35 AM

This is about to make GetAlreadyImportedOrNull to be a const function, as it should be naturally. This is a query function which should not initiate other imports.

balazske added inline comments.
lib/AST/ASTImporter.cpp
7741

This can be simplified by removing brace characters and removing ToD.

martong marked an inline comment as done.Nov 21 2018, 3:33 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
7741

Good catch, changed it.

martong updated this revision to Diff 174897.Nov 21 2018, 3:33 AM
martong marked an inline comment as done.
martong removed a reviewer: shafik.
martong removed a subscriber: gamesh411.
  • Better style without braces

@shafik , @gamesh411 I think when I used arcanist to update the patch it removed you as a reviewer and a subscriber. I am really sorry about this. Fortunately the Herald script added you guys back.
And once you are here, this is a gentle ping. :)

More constants are always welcome.

lib/AST/ExternalASTMerger.cpp
154 ↗(On Diff #174897)

Do we have to do the same for NamespaceDecls?

martong marked 2 inline comments as done.Nov 27 2018, 6:41 AM
martong added inline comments.
lib/AST/ExternalASTMerger.cpp
154 ↗(On Diff #174897)

Yes, I think so.
The primary context of a NamespaceDecl can be other than itself:

DeclContext *DeclContext::getPrimaryContext() {
  switch (DeclKind) {
  case Decl::TranslationUnit:
  case Decl::ExternCContext:
  case Decl::LinkageSpec:
  case Decl::Export:
  case Decl::Block:
  case Decl::Captured:
  case Decl::OMPDeclareReduction:
    // There is only one DeclContext for these entities.
    return this;

  case Decl::Namespace:
    // The original namespace is our primary context.
    return static_cast<NamespaceDecl *>(this)->getOriginalNamespace();

Consequently, we should call setMustBuildLookupTable only on a NamespaceDecl if we know for sure that is a primary context.

shafik requested changes to this revision.Nov 27 2018, 2:46 PM

The rest of the changes look good.

lib/AST/ExternalASTMerger.cpp
147 ↗(On Diff #174897)

This change looks unrelated to the refactoring of GetAlreadyImportedOrNull() so should probably be in another PR

154 ↗(On Diff #174897)

This change looks unrelated to the refactoring of GetAlreadyImportedOrNull() so should probably be in another PR

This revision now requires changes to proceed.Nov 27 2018, 2:46 PM
martong updated this revision to Diff 175639.Nov 28 2018, 1:36 AM
martong marked an inline comment as done.
  • Remove call of 'getPrimaryContext'

Hi @shafik ,

Thank you for your review. I have removed the call of getPrimaryContext.
Fortunately, we already have one patch for the getPrimaryContext related changes, made by @teemperor : https://reviews.llvm.org/D54898 . I have set that patch as the parent of this one (in the stack). Could you please take a look on that patch too?

As pointed out in this comment in another review

https://reviews.llvm.org/D44100#1311687

We need to make sure we are running the lldb test suite before committing and watching the bots to make sure the commit does not break them.

The change is not purely a refactor since previously the error was consumed.

Thank you

a_sidorin accepted this revision.Dec 1 2018, 12:23 PM

LGTM with a nit: the call to GetAlreadyImportedOrNull is not removed but moved into ImportDeclParts.

@shafik

We need to make sure we are running the lldb test suite before committing and watching the bots to make sure the commit does not break them.

I just have tested this patch on macOS and there seems to be no regression. Linux also seems to be without any regression. Very soon I am going to commit and I am going to monitor green.lab.llvm.org/green/view/LLDB/ for sign of any failure. I will revert in case of any failure asap.

Thanks for the review!

This revision was not accepted when it landed; it landed in state Needs Review.Dec 12 2018, 3:26 AM
This revision was automatically updated to reflect the committed changes.

Sounds good!