Page MenuHomePhabricator

[Support] Add overload writeFileAtomically(std::function Writer)
ClosedPublic

Authored by jkorous on Sep 10 2019, 5:21 PM.

Details

Summary

I noticed there are couple other implementations of atomic file write and replaced these with shared implementation.
I have to admit my enthusiasm for factoring this out waned a bit after I added proper error handling in lldb-platform and ThinLTOCodeGenerator as it doesn't look all that simpler than current code.
(Maybe I'm holding it wrong?) Open to suggestions.

Diff Detail

Repository
rL LLVM

Event Timeline

jkorous created this revision.Sep 10 2019, 5:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2019, 5:21 PM

Thanks Jan, I think this is a great improvement. IMHO the fact that we can share code outweighs the slightly increased complexity.

llvm/include/llvm/Support/FileUtilities.h
107 ↗(On Diff #219637)

I wonder if the temp path should be configurable or at least make it a default argument. Removing that would definitely make the call sites a little simpler. At least for LLDB I don't think the temp path matters.

gribozavr accepted this revision.Sep 11 2019, 3:13 AM
gribozavr added inline comments.
lldb/tools/lldb-server/lldb-platform.cpp
24 ↗(On Diff #219637)

Move the new header here, to group with the rest?

llvm/lib/Support/FileUtilities.cpp
332 ↗(On Diff #219637)

Please add a newline.

This revision is now accepted and ready to land.Sep 11 2019, 3:13 AM
jkorous marked 3 inline comments as done.Sep 12 2019, 2:11 PM
jkorous added inline comments.
llvm/include/llvm/Support/FileUtilities.h
107 ↗(On Diff #219637)

Of course, good point! I'll create a separate patch.

This revision was automatically updated to reflect the committed changes.