This is an archive of the discontinued LLVM Phabricator instance.

[CTU] Make loadExternalAST return with non nullptr on success
ClosedPublic

Authored by martong on Dec 4 2018, 9:40 AM.

Diff Detail

Repository
rC Clang

Event Timeline

martong created this revision.Dec 4 2018, 9:40 AM

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
194

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

balazske added inline comments.Dec 5 2018, 2:23 AM
include/clang/CrossTU/CrossTranslationUnit.h
134

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

lib/CrossTU/CrossTranslationUnit.cpp
194

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

324

Add explanation text to the assert?

martong updated this revision to Diff 176779.Dec 5 2018, 2:45 AM
  • Return an error from loadExternalAST in case of nullptr
This revision is now accepted and ready to land.Dec 5 2018, 2:49 AM

@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 retitled this revision from [CTU] Remove redundant error check to [CTU] Make loadExternalAST return with non nullptr on success.Dec 5 2018, 2:53 AM
martong edited the summary of this revision. (Show Details)
martong marked an inline comment as done.Dec 5 2018, 8:42 AM
martong added inline comments.
lib/CrossTU/CrossTranslationUnit.cpp
194

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 requested changes to this revision.Dec 5 2018, 1:24 PM

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.

This revision now requires changes to proceed.Dec 5 2018, 1:24 PM

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.

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

a_sidorin accepted this revision.Dec 23 2018, 1:23 AM

Hi Gabor,
Yes, this looks good. Thanks!

This revision is now accepted and ready to land.Dec 23 2018, 1:23 AM
This revision was automatically updated to reflect the committed changes.