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

[Rui Ueyama via llvm-commits]

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.

> +// A FileOutputBuffer which keeps data in memory and writes to the final
> > +// output file on commit(). This is used only when we cannot use
> OnDiskBuffer.
> > +class InMemoryBuffer : public FileOutputBuffer {
> > +public:
> > +  InMemoryBuffer(StringRef Path, MemoryBlock Buf, unsigned Mode)
> > +      : FileOutputBuffer(Path), Buffer(Buf), Mode(Mode) {}
> > +
> > +  static ErrorOr<std::unique_ptr<InMemoryBuffer>>
> > +  create(StringRef Path, size_t Size, unsigned Mode) {
>
> Same here. Why both a public constructor and create?
>
> > +  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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171106/444d0db4/attachment.html>