[PATCH] D113226: Support: Change FileOutputBuffer to return FileError

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113226/new/

https://reviews.llvm.org/D113226