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

[Rafael Avila de Espindola via llvm-commits]

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?

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

Cheers,
Rafael