This is an archive of the discontinued LLVM Phabricator instance.

Fix printing policy for AST context loaded from file
ClosedPublic

Authored by jklaehn on Jul 11 2017, 11:22 AM.

Details

Summary

In ASTUnit::LoadFromASTFile, the context object is set up using default-constructed
LangOptions (which only later get populated). As the language options are used in the constructor
of PrintingPolicy, this needs to be updated explicitly after the language options are available.

Diff Detail

Event Timeline

jklaehn created this revision.Jul 11 2017, 11:22 AM
vsk added a subscriber: vsk.Jul 13 2017, 7:42 PM

I wonder if it's possible to do away with the calls to 'updated()'... it seems strange that we initialize the same preprocessor repeatedly. Is there any way to finalize an ASTInfoCollector after ReadAST happens (or ASTReaderListeners in general)?

In D35271#809159, @vsk wrote:

I wonder if it's possible to do away with the calls to 'updated()'... it seems strange that we initialize the same preprocessor repeatedly. Is there any way to finalize an ASTInfoCollector after ReadAST happens (or ASTReaderListeners in general)?

I can look into this but would prefer to do so in a different patch, as this would require refactoring beyond this simple bug fix. Would it be okay to land this patch as-is?

vsk added a comment.Aug 21 2017, 11:47 AM
In D35271#809159, @vsk wrote:

I wonder if it's possible to do away with the calls to 'updated()'... it seems strange that we initialize the same preprocessor repeatedly. Is there any way to finalize an ASTInfoCollector after ReadAST happens (or ASTReaderListeners in general)?

I can look into this but would prefer to do so in a different patch, as this would require refactoring beyond this simple bug fix. Would it be okay to land this patch as-is?

Not having worked on this code I'm afraid I can't say. Usually it's a good idea to include a regression test.

jklaehn updated this revision to Diff 112139.Aug 22 2017, 3:40 AM
jklaehn added a reviewer: akyrtzi.

Added regression test.

vsk added a comment.Aug 22 2017, 11:23 AM

Thanks for adding the test! This is looking good.

unittests/Frontend/ASTUnitTest.cpp
32 ↗(On Diff #112139)

If a directory isn't necessary to exercise the desired functionality, it'd be better to use createTemporaryFile() to open a temporary file descriptor, and then to use tool_output_file() to manage the descriptor's lifetime. Tool_output_file has logic to clean up files when a signal is received, etc.

48 ↗(On Diff #112139)

It doesn't look like all of the directories which could be created here would be deleted? With any luck it'll be possible to use tool_output_file() to side step the issue.

jklaehn updated this revision to Diff 112334.Aug 23 2017, 5:10 AM

Update regression test to use createTemporaryFile() and tool_output_file as suggested.

vsk accepted this revision.Aug 23 2017, 10:31 AM

Thanks, LGTM! This seems like a pretty straightforward bug fix. Since it's not my usual area maybe it'd be worth waiting a day or so for more feedback.

This revision is now accepted and ready to land.Aug 23 2017, 10:31 AM
jklaehn marked 2 inline comments as done.Aug 25 2017, 12:21 AM
In D35271#850472, @vsk wrote:

Thanks, LGTM! This seems like a pretty straightforward bug fix. Since it's not my usual area maybe it'd be worth waiting a day or so for more feedback.

Thanks! I do not have commit access so it would be great if you could commit it on my behalf.

This revision was automatically updated to reflect the committed changes.