Page MenuHomePhabricator

[ASTImporter] Propagate error from ImportDeclContext

Authored by martong on Jun 20 2019, 7:39 AM.



During analysis of one project we failed to import one
CXXDestructorDecl. But since we did not propagate the error in
importDeclContext we had a CXXRecordDecl without a destructor. Then the
analyzer engine had a CallEvent where the nonexistent dtor was requested

Solution is to propagate the errors we have during importing a

Diff Detail


Event Timeline

martong created this revision.Jun 20 2019, 7:39 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

I don't have the insight to LGTM the whole change - just a nit about implementation detail.

1724 ↗(On Diff #205810)

You might consider using just a lambda with llvm::make_scope_exit.

For example:

auto DefinitionCompleter = llvm::make_scope_exit( [To](){To->completeDefinition();} );
martong updated this revision to Diff 206018.Jun 21 2019, 9:17 AM
  • Remove formatv b/c it can't handle braces in code
balazske added inline comments.Jun 24 2019, 12:59 AM
4743 ↗(On Diff #206018)

Maybe add a similar test for namespace and check that the error is not propagated?

martong updated this revision to Diff 206223.Jun 24 2019, 8:31 AM
  • Remove the macro and use std::string.op+
martong marked 4 inline comments as done.Jun 24 2019, 9:59 AM
martong added inline comments.
1724 ↗(On Diff #205810)

Thanks! I have changed to make_scope_exit.

4743 ↗(On Diff #206018)

Ok, I have added that test.

martong updated this revision to Diff 206250.Jun 24 2019, 9:59 AM
martong marked 2 inline comments as done.
  • Use make_scope_exit
  • Add test ErrorIsNotPropagatedFromMemberToNamespace
a_sidorin accepted this revision.Jun 30 2019, 9:53 AM

Hi Gabor,
The patch looks good. Thanks!

This revision is now accepted and ready to land.Jun 30 2019, 9:53 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2019, 5:44 AM

@shafik I've been looking for any lldb regression in our Mac machine, could not find any. Now I am looking at . I don't expect regression because here we changed logic about the error handling only, so I'd expect to have regression only if we had testcases in lldb for erroneous cases, but apparently there are no such tests.