[CTU] Make loadExternalAST return with non nullptr on success

[martong]

Summary

Change loadExternalAST to return with either an error or with a valid
ASTUnit pointer which should not be a nullptr.

[Szelethus]

From what I can see, this patch LGTM, but I lack the experience in the CTU department just yet to give any meaningful feedback.

lib/CrossTU/CrossTranslationUnit.cpp

149

Would it be worth to add a comment that this function never returns with nullptr on success?

[balazske]

include/clang/CrossTU/CrossTranslationUnit.h

122

Return value description still not accurate: It returns the Expected object, not the pointer itself.

lib/CrossTU/CrossTranslationUnit.cpp

149

Even on failure not. Failure should be Error, success is a non-null pointer.

240

Add explanation text to the assert?

[martong]

• Return an error from loadExternalAST in case of nullptr

[xazax.hun]

LG!

[martong]

@Szelethus @balazske Thanks for your review! The meantime I have discussed with @xazax.hun that actually the called ASTUnit::LoadFromASTFile function inside loadExternalAST may return with a nullptr. So, the best is to handle that inside loadExternalAST.

[martong]

lib/CrossTU/CrossTranslationUnit.cpp

149

Yes, you are right.
We should indicate that in the documentation of this function that it can not return with a nullptr on success. I think we should do that in here https://reviews.llvm.org/D55131 because there we do check that the ToDecl is not a nullptr.

[a_sidorin]

Hi Gabor,
There is a code in getExternalAST:

std::unique_ptr<ASTUnit> LoadedUnit(ASTUnit::LoadFromASTFile(
    ASTFileName, CI.getPCHContainerOperations()->getRawReader(),
    ASTUnit::LoadEverything, Diags, CI.getFileSystemOpts()));
Unit = LoadedUnit.get();
FileASTUnitMap[ASTFileName] = std::move(LoadedUnit);

And ASTUnit::LoadFromASTFile()can return nullptr. Actually, there is a problem in loadExternalAST() - it ignores this fact.

[martong]

Hi Aleksey,

The first version was indeed not correct. But, I think we handle that case with the latest update (https://reviews.llvm.org/differential/diff/176779/). In loadExternalAST() we have this right before the return:

if (!Unit)
    return llvm::make_error<IndexError>(
        index_error_code::failed_to_get_external_ast);
return Unit;

So we will never return with a nullptr value in the Expected.

[martong]

Ping @a_sidorin Could you please take another look and consider my previous comment?

[a_sidorin]

Hi Gabor,
Yes, this looks good. Thanks!

include/clang/CrossTU/CrossTranslationUnit.h

public:
   /// This function loads a function definition from an external AST
   ///        file.
   ///
   /// A function definition with the same declaration will be looked up in the
   /// index file which should be in the \p CrossTUDir directory, called
   /// \p IndexName. In case the declaration is found in the index the
   /// corresponding AST file will be loaded.
   ///
-  /// \return Returns an ASTUnit that contains the definition of the looked up
-  /// function.
+  /// \return Returns a pointer to the ASTUnit that contains the definition of
+  /// the looked up function or an Error.
+  /// The returned pointer is never a nullptr.
   ///
   /// Note that the AST files should also be in the \p CrossTUDir.
   llvm::Expected<ASTUnit *> loadExternalAST(StringRef LookupName,
                                             StringRef CrossTUDir,
                                             StringRef IndexName);
 
   /// This function merges a definition from a separate AST Unit into
   ///        the current one which was created by the compiler instance that

lib/CrossTU/CrossTranslationUnit.cpp

if (!ND || !ND->hasBody(ResultDecl))
       continue;
     if (getLookupName(ResultDecl) != LookupFnName)
       continue;
     return ResultDecl;
   }
   return nullptr;
 }
 
 llvm::Expected<const FunctionDecl *>
 CrossTranslationUnitContext::getCrossTUDefinition(const FunctionDecl *FD,
                                                   StringRef CrossTUDir,
                                                   StringRef IndexName) {
   assert(!FD->hasBody() && "FD has a definition in current translation unit!");
   const std::string LookupFnName = getLookupName(FD);
   if (LookupFnName.empty())
     return llvm::make_error<IndexError>(
         index_error_code::failed_to_generate_usr);
   llvm::Expected<ASTUnit *> ASTUnitOrError =
       loadExternalAST(LookupFnName, CrossTUDir, IndexName);
   if (!ASTUnitOrError)
     return ASTUnitOrError.takeError();
   ASTUnit *Unit = *ASTUnitOrError;
-  if (!Unit)
-    return llvm::make_error<IndexError>(
-        index_error_code::failed_to_get_external_ast);
   assert(&Unit->getFileManager() ==
          &Unit->getASTContext().getSourceManager().getFileManager());
 
   TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
   if (const FunctionDecl *ResultDecl =
           findFunctionInDeclContext(TU, LookupFnName))
     return importDefinition(ResultDecl);
   return llvm::make_error<IndexError>(index_error_code::failed_import);
if (ASTCacheEntry == FileASTUnitMap.end()) {
       FileASTUnitMap[ASTFileName] = std::move(LoadedUnit);
     } else {
       Unit = ASTCacheEntry->second.get();
     }
     FunctionASTUnitMap[LookupName] = Unit;
   } else {
     Unit = FnUnitCacheEntry->second;
   }
+  if (!Unit)
+    return llvm::make_error<IndexError>(
+        index_error_code::failed_to_get_external_ast);
   return Unit;
 }
 
 llvm::Expected<const FunctionDecl *>
 CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD) {
   ASTImporter &Importer = getOrCreateASTImporter(FD->getASTContext());
   auto *ToDecl =
       cast<FunctionDecl>(Importer.Import(const_cast<FunctionDecl *>(FD)));