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

Rui Ueyama via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 6 12:44:49 PST 2017


On Mon, Nov 6, 2017 at 12:32 PM, Rafael Avila de Espindola <
rafael.espindola at gmail.com> wrote:

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


Maybe it makes more sense if we make these functions non-member functions,
so that they don't look like they have both public constructors and factory
methods. I'll try to do that.

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

 Agreed. I don't have a plan to use InMemoryBuffer when a filesystem
doesn't support block pre-allocation.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171106/ae146026/attachment.html>


More information about the llvm-commits mailing list