[ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[martong]

Summary

a_sidorin

[martong]

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]

lib/AST/ASTImporter.cpp

7741

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

[martong]

lib/AST/ASTImporter.cpp

7741

Good catch, changed it.

[martong]

• Better style without braces

[martong]

@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. :)

[a_sidorin]

More constants are always welcome.

lib/AST/ExternalASTMerger.cpp

154 ↗

(On Diff #174897)

Do we have to do the same for NamespaceDecls?

[martong]

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]

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

[martong]

• Remove call of 'getPrimaryContext'

[martong]

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?

[shafik]

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]

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

[martong]

@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!

[shafik]

Sounds good!

include/clang/AST/ASTImporter.h

public:
     Decl *Import(Decl *FromD);
     Decl *Import(const Decl *FromD) {
       return Import(const_cast<Decl *>(FromD));
     }
 
     /// Return the copy of the given declaration in the "to" context if
     /// it has already been imported from the "from" context.  Otherwise return
     /// NULL.
-    Decl *GetAlreadyImportedOrNull(Decl *FromD);
+    Decl *GetAlreadyImportedOrNull(const Decl *FromD) const;
 
     /// Import the given declaration context from the "from"
     /// AST context into the "to" AST context.
     ///
     /// \returns the equivalent declaration context in the "to"
     /// context, or error value.
     llvm::Expected<DeclContext *> ImportContext(DeclContext *FromDC);
 

lib/AST/ASTImporter.cpp

Error ASTNodeImporter::ImportDeclParts(
   if (Error Err = importInto(Name, D->getDeclName()))
     return Err;
 
   // Import the location of this declaration.
   if (Error Err = importInto(Loc, D->getLocation()))
     return Err;
 
   ToD = cast_or_null<NamedDecl>(Importer.GetAlreadyImportedOrNull(D));
+  if (ToD)
+    if (Error Err = ASTNodeImporter(*this).ImportDefinitionIfNeeded(D, ToD))
+      return Err;
 
   return Error::success();
 }
 
 Error ASTNodeImporter::ImportDefinitionIfNeeded(Decl *FromD, Decl *ToD) {
   if (!FromD)
     return Error::success();
 

 }
 
 Attr *ASTImporter::Import(const Attr *FromAttr) {
   Attr *ToAttr = FromAttr->clone(ToContext);
   ToAttr->setRange(Import(FromAttr->getRange()));
   return ToAttr;
 }
 
-Decl *ASTImporter::GetAlreadyImportedOrNull(Decl *FromD) {
-  llvm::DenseMap<Decl *, Decl *>::iterator Pos = ImportedDecls.find(FromD);
-  if (Pos != ImportedDecls.end()) {
-    Decl *ToD = Pos->second;
-    // FIXME: move this call to ImportDeclParts().
-    if (Error Err = ASTNodeImporter(*this).ImportDefinitionIfNeeded(FromD, ToD))
-      llvm::consumeError(std::move(Err));
-    return ToD;
-  } else {
+Decl *ASTImporter::GetAlreadyImportedOrNull(const Decl *FromD) const {
+  auto Pos = ImportedDecls.find(FromD);
+  if (Pos != ImportedDecls.end())
+    return Pos->second;
+  else
     return nullptr;
-  }
-}
 
 Decl *ASTImporter::Import(Decl *FromD) {
   if (!FromD)
     return nullptr;
 
   ASTNodeImporter Importer(*this);
 
   // Check whether we've already imported this declaration.