<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Nov 6, 2017 at 12:32 PM, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> writes:<br>
<br>
> On Mon, Nov 6, 2017 at 11:25 AM, Rafael Avila de Espindola <<br>
> <a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
><br>
>> Rui Ueyama via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> writes:<br>
>><br>
>> > +// A FileOutputBuffer which creates a temporary file in the same<br>
>> directory<br>
>> > +// as the final output file. The final output file is atomically<br>
>> replaced<br>
>> > +// with the temporary file on commit().<br>
>> > +class OnDiskBuffer : public FileOutputBuffer {<br>
>> > +public:<br>
>> > +  OnDiskBuffer(StringRef Path, StringRef TempPath,<br>
>> > +               std::unique_ptr<fs::mapped_<wbr>file_region> Buf)<br>
>> > +      : FileOutputBuffer(Path), Buffer(std::move(Buf)),<br>
>> TempPath(TempPath) {}<br>
>> > +  static ErrorOr<std::unique_ptr<<wbr>OnDiskBuffer>><br>
>> > +  create(StringRef Path, size_t Size, unsigned Mode);<br>
>><br>
>> Why do you need both a public constructor and a create method? Can the<br>
>> constructor be private?<br>
><br>
><br>
> I couldn't use llvm::make_unique without making the constructor public.<br>
<br>
</span>That is a small annoyance with make_unique. I wonder if it will work on<br>
some new c++ version.<br>
<br>
Given that we don't use exception I think I would use new, but that is<br>
not a big issue.</blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>> > +  std::error_code commit() override {<br>
>> > +    int FD;<br>
>> > +    std::error_code EC;<br>
>> > +    if (auto EC = openFileForWrite(FinalPath, FD, fs::F_None, Mode))<br>
>> > +      return EC;<br>
>> > +    raw_fd_ostream OS(FD, /*shouldClose=*/true, /*unbuffered=*/true);<br>
>> > +    OS << StringRef((const char *)Buffer.base(), Buffer.size());<br>
>> > +    return std::error_code();<br>
>><br>
>> This is currently correct as this method is never used with regular<br>
>> files.<br>
>><br>
>> Is the idea to use InMemoryBuffer for where we don't have a way of<br>
>> allocating an output file? If so when we do that this should still use a<br>
>> temporary file if the output is a regular file.<br>
>><br>
><br>
> Can you elaborate?<br>
><br>
> Currently, if you cannot create a temporary file in the same directory as<br>
> the output file, FileOutputBuffer creates an in-memory buffer instead of<br>
> creating a temporary file in a temporary directory (e.g. /tmp), as there's<br>
> no guarantee that the temporary directory and the destination directory are<br>
> on the same filesystem.<br>
<br>
</span>Sure. Currently (as of this patch), the use is fine. The issue is only<br>
if the intention is to use InMemoryBuffer for filesystems that don't<br>
support allocating space. In that case we should still use a temporary<br>
file + rename.<br></blockquote><div><br></div><div> Agreed. I don't have a plan to use InMemoryBuffer when a filesystem doesn't support block pre-allocation.</div></div></div></div>