Page MenuHomePhabricator

Support: Change FileOutputBuffer to return FileError
Needs ReviewPublic

Authored by dexonsmith on Thu, Nov 4, 4:15 PM.

Details

Reviewers
dblaikie
lhames
MaskRay
gkm
Group Reviewers
Restricted Project
Summary

Update FileOutputBuffer to always return a FileError, now that it will
pass through the error code from the wrapped error. Also update the
callers not to redundantly include the filename in messages.

One awkward question is whether sys::fs::TempFile should return
FileError. If TempFile starts doing so and it reports the temporary file
path, then FileOutputBuffer should stop adding one... but it probably
wants to avoid leaking the temporary name and report the intended output
file instead. Maybe createFileError() should avoid double-wrapping a
FileError, instead replacing the named file in that error?

Another awkward question is whether all the callers were updated to
avoid redundantly reporting the filename. No tests failed, even before I
audited the callers. Maybe this is fine?

Note: this is an example of using https://reviews.llvm.org/D113225.

Diff Detail

Event Timeline

dexonsmith created this revision.Thu, Nov 4, 4:15 PM
Herald added a reviewer: gkm. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
dexonsmith requested review of this revision.Thu, Nov 4, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Nov 4, 4:15 PM
Herald added a subscriber: aheejin. · View Herald Transcript

Maybe createFileError() should avoid double-wrapping a FileError, instead replacing the named file in that error?

Got examples of what the double-wrapping looks like? Maybe compared to how it looks with other tools that might encounter similar issues?

I'd guess changing file names could result in incorrect messages, though - suggesting that, say, a directory is read-only but the final target directory isn't read only and it's really an issue with the temp file location - so maybe it'd be best to keep the double wrapping?

Another awkward question is whether all the callers were updated to avoid redundantly reporting the filename. No tests failed, even before I audited the callers. Maybe this is fine?

Better test coverage is always nice - but I'll admit I can be a bit lax on error paths. Maybe changing the error paths to "assert(false)" could help identify tests for them (hopefully there are some) that could be fleshed out to check the actual error message more narrowly.