Page MenuHomePhabricator

[Debugger] Use FileSystem instead of calling llvm::sys::fs::openFileForWrite directly.
ClosedPublic

Authored by JDevlieghere on Jun 9 2020, 1:08 PM.

Details

Summary

This replaces the (only) call to llvm::sys::fs::openFileForWrite with FileSystem::Open. This guarantees that we include log files in the reproducers.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jun 9 2020, 1:08 PM

This looks okay to me though I'm not very familiar with the llvm file system interfaces.

Do we have any tests that tests that log output gets emitted to the file requested when you do "log enable -f somefile whatever"? If so and they still work, LGTM. If we don't test that at all, we should probably add a test on the principle of "when you monkey with something that doesn't have a test, you should add one..."

This looks okay to me though I'm not very familiar with the llvm file system interfaces.

Do we have any tests that tests that log output gets emitted to the file requested when you do "log enable -f somefile whatever"? If so and they still work, LGTM. If we don't test that at all, we should probably add a test on the principle of "when you monkey with something that doesn't have a test, you should add one..."

Yep, we had two tests that were screaming at me when I messed up :-)

jingham accepted this revision.Jun 10 2020, 3:38 PM

LGTM then.

This revision is now accepted and ready to land.Jun 10 2020, 3:38 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2020, 6:21 PM