[llvm] r317127 - Rewrite FileOutputBuffer as two separate classes.

[Rafael Avila de Espindola via llvm-commits]

Rui Ueyama <ruiu at google.com> writes:

> On Mon, Nov 6, 2017 at 11:25 AM, Rafael Avila de Espindola <
> rafael.espindola at gmail.com> wrote:
>
>> Rui Ueyama via llvm-commits <llvm-commits at lists.llvm.org> writes:
>>
>> > +// A FileOutputBuffer which creates a temporary file in the same
>> directory
>> > +// as the final output file. The final output file is atomically
>> replaced
>> > +// with the temporary file on commit().
>> > +class OnDiskBuffer : public FileOutputBuffer {
>> > +public:
>> > +  OnDiskBuffer(StringRef Path, StringRef TempPath,
>> > +               std::unique_ptr<fs::mapped_file_region> Buf)
>> > +      : FileOutputBuffer(Path), Buffer(std::move(Buf)),
>> TempPath(TempPath) {}
>> > +  static ErrorOr<std::unique_ptr<OnDiskBuffer>>
>> > +  create(StringRef Path, size_t Size, unsigned Mode);
>>
>> Why do you need both a public constructor and a create method? Can the
>> constructor be private?
>
>
> I couldn't use llvm::make_unique without making the constructor public.

That is a small annoyance with make_unique. I wonder if it will work on
some new c++ version.

Given that we don't use exception I think I would use new, but that is
not a big issue.

>> > +  std::error_code commit() override {
>> > +    int FD;
>> > +    std::error_code EC;
>> > +    if (auto EC = openFileForWrite(FinalPath, FD, fs::F_None, Mode))
>> > +      return EC;
>> > +    raw_fd_ostream OS(FD, /*shouldClose=*/true, /*unbuffered=*/true);
>> > +    OS << StringRef((const char *)Buffer.base(), Buffer.size());
>> > +    return std::error_code();
>>
>> This is currently correct as this method is never used with regular
>> files.
>>
>> Is the idea to use InMemoryBuffer for where we don't have a way of
>> allocating an output file? If so when we do that this should still use a
>> temporary file if the output is a regular file.
>>
>
> Can you elaborate?
>
> Currently, if you cannot create a temporary file in the same directory as
> the output file, FileOutputBuffer creates an in-memory buffer instead of
> creating a temporary file in a temporary directory (e.g. /tmp), as there's
> no guarantee that the temporary directory and the destination directory are
> on the same filesystem.

Sure. Currently (as of this patch), the use is fine. The issue is only
if the intention is to use InMemoryBuffer for filesystems that don't
support allocating space. In that case we should still use a temporary
file + rename.

Cheers,
Rafael