This is an archive of the discontinued LLVM Phabricator instance.

[index-while-buildling] FSUtil
ClosedPublic

Authored by jkorous on Aug 27 2019, 6:43 PM.

Diff Detail

Event Timeline

jkorous created this revision.Aug 27 2019, 6:43 PM
gribozavr added inline comments.Aug 28 2019, 7:31 AM
clang/lib/IndexSerialization/FSUtil.h
27 ↗(On Diff #217562)

"writeFileAtomically"

28 ↗(On Diff #217562)

Please use StringRef instead of SmallString.

29 ↗(On Diff #217562)

Why not llvm::Error?

35 ↗(On Diff #217562)

failed to create a temporary file with model '...'

Traditionally after a colon one would write the reason why the operation failed, like "not enough space on disk"

62 ↗(On Diff #217562)

Why not put it into llvm/Support/FileUtilities.h?

jkorous marked 5 inline comments as done.Aug 28 2019, 1:43 PM

Good points as always. Thanks!

jkorous updated this revision to Diff 217724.Aug 28 2019, 1:46 PM
jkorous edited the summary of this revision. (Show Details)

Addressed comments.

Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2019, 1:46 PM

Hmm, actually if we replace Buffer in the interface with a llvm::function_ref<void(llvm::raw_ostream &)> callback it would cover couple other existing uses of createUniqueFile.

gribozavr accepted this revision.Aug 29 2019, 2:29 AM

Hmm, actually if we replace Buffer in the interface with a llvm::function_ref<void(llvm::raw_ostream &)> callback it would cover couple other existing uses of createUniqueFile.

That would make a lot of sense if you're going to do the cleanup and refactor those callers to use new API. Good idea!

However, I think just from the API simplicity perspective, it would make sense to have two overloaded APIs if calling this function with a precomputed buffer is common.

This revision is now accepted and ready to land.Aug 29 2019, 2:29 AM

Sorry for the late addition to the review, but any chance this API could have a unit test? Of course we can't test the automicity, but basic end to end flow on the "happy path" would be nice to test.

jkorous updated this revision to Diff 218811.Sep 4 2019, 4:46 PM

Add test.

Hmm, actually if we replace Buffer in the interface with a llvm::function_ref<void(llvm::raw_ostream &)> callback it would cover couple other existing uses of createUniqueFile.

That would make a lot of sense if you're going to do the cleanup and refactor those callers to use new API. Good idea!

However, I think just from the API simplicity perspective, it would make sense to have two overloaded APIs if calling this function with a precomputed buffer is common.

Ok. I'll land this simple version first and will take a look at the overload next week.

This revision was automatically updated to reflect the committed changes.