Page MenuHomePhabricator

[ASTImporter] Corrected diagnostic client handling in tests.
ClosedPublic

Authored by balazske on May 28 2018, 1:43 AM.

Details

Summary

ASTImporter tests may produce source file related warnings, the diagnostic
client should be in correct state to handle it. Added 'beginSourceFile' to set
the client state.

Diff Detail

Repository
rC Clang

Event Timeline

balazske created this revision.May 28 2018, 1:43 AM

Adding Argyrios and Daniel as a reviewer for ASTUnit related changes.

martong accepted this revision.May 28 2018, 5:38 AM

LGTM! But let's wait the review of those people who have history in ASTUnit.cpp.

This revision is now accepted and ready to land.May 28 2018, 5:38 AM
a.sidorin accepted this revision.May 28 2018, 5:41 AM

Looks good to me, but the approval from AST code owners is required, I think.

It's not clear to me what kind of issue you are addressing, for example is the unit test hitting an assertion hit without your changes ? Or is there something else ?
Also it would be good to add some documentation comments to clarify what's the use case and when ASTUnit::beginSourceFile() should be getting called.

balazske added a comment.EditedMay 31 2018, 12:43 AM

If BeginSourceFile is not called on the diagnostic client object, it is not possible to have compiler warnings or errors that come from the "To" context while importing something (there is assertion in TextDiagnosticPrinter::HandleDiagnostic). In the currently existing tests there is no such case but I have not yet committed tests that produce such warnings (for example ODR violations during import or encounter of unsupported constructs).

The new ASTUnit::beginSourceFile is only there to simplify the code. It is possible to get Ctx, PP and getDiagnostic() from outside of ASTUnit and call the same thing, but requires more code to write. Probably a more smart place to call BeginSourceFile can be found if we look deep into ASTUnit and buildAST functions that generate the AST from code that has associated source file but this would be a change that affects more code.

Thanks for the explanation. Please do add documentation comments for the new method so people using ASTUnit in their own code have an idea when and why they would need to call this. Something like "if you intend to emit additional diagnostics after the ASTUnit is created [...]".
Also consider making the naming more clear to match the intended purpose, like enableEmittingAdditionalDiagnostics() or something similar.
Otherwise LGTM.

balazske updated this revision to Diff 149412.Jun 1 2018, 2:13 AM
  • Added comment, renamed beginSourceFile, removed check for PP.

Check for PP is removed because it is allowed to be nullptr.

From API point of view if there is a enableSourceFileDiagnostics there should be a disableSourceFileDiagnostics too (that calls the EndSourceFile). But I am not sure how and if to use it at all. In the unit tests it is not needed, the ASTUnit contains a single entity for the "To" context.

We could leave disableSourceFileDiagnostics off until someone finds a use case for it.

I want this change to be committed by somebody. I have no commit right.

This revision was automatically updated to reflect the committed changes.