[PATCH] Use ErrorOr for the ::create factory on instrumented and sample profilers.

[Justin Bogner]

Diego Novillo <dnovillo at google.com> writes:
> Hi bogner,
>
> As discussed in
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20141027/242445.html,the creation of reader and writer instances is better done using
> ErrorOr. There are no functional changes, but several callers needed to
> be adjusted.

Thanks for doing this! LGTM.

> http://reviews.llvm.org/D6076
>
> Files:
>   include/llvm/ProfileData/InstrProfReader.h
>   include/llvm/ProfileData/SampleProfReader.h
>   include/llvm/ProfileData/SampleProfWriter.h
>   lib/ProfileData/InstrProfReader.cpp
>   lib/ProfileData/SampleProfReader.cpp
>   lib/ProfileData/SampleProfWriter.cpp
>   lib/Transforms/Scalar/SampleProfile.cpp
>   tools/llvm-profdata/llvm-profdata.cpp
>
> Index: include/llvm/ProfileData/InstrProfReader.h
> ===================================================================
> --- include/llvm/ProfileData/InstrProfReader.h
> +++ include/llvm/ProfileData/InstrProfReader.h
> @@ -18,6 +18,7 @@
>  #include "llvm/ADT/ArrayRef.h"
>  #include "llvm/ADT/StringExtras.h"
>  #include "llvm/ProfileData/InstrProf.h"
> +#include "llvm/Support/ErrorOr.h"
>  #include "llvm/Support/LineIterator.h"
>  #include "llvm/Support/MemoryBuffer.h"
>  #include "llvm/Support/EndianStream.h"
> @@ -94,8 +95,7 @@
>  
>    /// Factory method to create an appropriately typed reader for the given
>    /// instrprof file.
> -  static std::error_code create(std::string Path,
> -                                std::unique_ptr<InstrProfReader> &Result);
> +  static ErrorOr<std::unique_ptr<InstrProfReader>> create(std::string Path);
>  };
>  
>  /// Reader for the simple text based instrprof format.
> Index: include/llvm/ProfileData/SampleProfReader.h
> ===================================================================
> --- include/llvm/ProfileData/SampleProfReader.h
> +++ include/llvm/ProfileData/SampleProfReader.h
> @@ -92,9 +92,8 @@
>    }
>  
>    /// \brief Create a sample profile reader appropriate to the file format.
> -  static std::error_code create(StringRef Filename,
> -                                std::unique_ptr<SampleProfileReader> &Reader,
> -                                LLVMContext &C);
> +  static ErrorOr<std::unique_ptr<SampleProfileReader>>
> +  create(StringRef Filename, LLVMContext &C);
>  
>  protected:
>    /// \brief Map every function to its associated profile.
> Index: include/llvm/ProfileData/SampleProfWriter.h
> ===================================================================
> --- include/llvm/ProfileData/SampleProfWriter.h
> +++ include/llvm/ProfileData/SampleProfWriter.h
> @@ -17,6 +17,7 @@
>  #include "llvm/IR/Function.h"
>  #include "llvm/IR/Module.h"
>  #include "llvm/ProfileData/SampleProf.h"
> +#include "llvm/Support/ErrorOr.h"
>  #include "llvm/Support/FileSystem.h"
>  #include "llvm/Support/raw_ostream.h"
>  
> @@ -71,9 +72,8 @@
>  
>    /// \brief Profile writer factory. Create a new writer based on the value of
>    /// \p Format.
> -  static std::error_code create(StringRef Filename,
> -                                std::unique_ptr<SampleProfileWriter> &Result,
> -                                SampleProfileFormat Format);
> +  static ErrorOr<std::unique_ptr<SampleProfileWriter>>
> +  create(StringRef Filename, SampleProfileFormat Format);
>  
>  protected:
>    /// \brief Output stream where to emit the profile to.
> Index: lib/ProfileData/InstrProfReader.cpp
> ===================================================================
> --- lib/ProfileData/InstrProfReader.cpp
> +++ lib/ProfileData/InstrProfReader.cpp
> @@ -21,32 +21,34 @@
>  
>  using namespace llvm;
>  
> -static std::error_code
> -setupMemoryBuffer(std::string Path, std::unique_ptr<MemoryBuffer> &Buffer) {
> +static ErrorOr<std::unique_ptr<MemoryBuffer>>
> +setupMemoryBuffer(std::string Path) {
>    ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
>        MemoryBuffer::getFileOrSTDIN(Path);
>    if (std::error_code EC = BufferOrErr.getError())
>      return EC;
> -  Buffer = std::move(BufferOrErr.get());
> +  auto Buffer = std::move(BufferOrErr.get());
>  
>    // Sanity check the file.
>    if (Buffer->getBufferSize() > std::numeric_limits<unsigned>::max())
>      return instrprof_error::too_large;
> -  return instrprof_error::success;
> +  return std::move(Buffer);
>  }
>  
>  static std::error_code initializeReader(InstrProfReader &Reader) {
>    return Reader.readHeader();
>  }
>  
> -std::error_code
> -InstrProfReader::create(std::string Path,
> -                        std::unique_ptr<InstrProfReader> &Result) {
> +ErrorOr<std::unique_ptr<InstrProfReader>>
> +InstrProfReader::create(std::string Path) {
>    // Set up the buffer to read.
> -  std::unique_ptr<MemoryBuffer> Buffer;
> -  if (std::error_code EC = setupMemoryBuffer(Path, Buffer))
> +  auto BufferOrError = setupMemoryBuffer(Path);
> +  if (std::error_code EC = BufferOrError.getError())
>      return EC;
>  
> +  auto Buffer = std::move(BufferOrError.get());
> +  std::unique_ptr<InstrProfReader> Result;
> +
>    // Create the reader.
>    if (IndexedInstrProfReader::hasFormat(*Buffer))
>      Result.reset(new IndexedInstrProfReader(std::move(Buffer)));
> @@ -58,16 +60,20 @@
>      Result.reset(new TextInstrProfReader(std::move(Buffer)));
>  
>    // Initialize the reader and return the result.
> -  return initializeReader(*Result);
> +  if (std::error_code EC = initializeReader(*Result))
> +    return EC;
> +
> +  return std::move(Result);
>  }
>  
>  std::error_code IndexedInstrProfReader::create(
>      std::string Path, std::unique_ptr<IndexedInstrProfReader> &Result) {
>    // Set up the buffer to read.
> -  std::unique_ptr<MemoryBuffer> Buffer;
> -  if (std::error_code EC = setupMemoryBuffer(Path, Buffer))
> +  auto BufferOrError = setupMemoryBuffer(Path);
> +  if (std::error_code EC = BufferOrError.getError())
>      return EC;
>  
> +  auto Buffer = std::move(BufferOrError.get());
>    // Create the reader.
>    if (!IndexedInstrProfReader::hasFormat(*Buffer))
>      return instrprof_error::bad_magic;
> Index: lib/ProfileData/SampleProfReader.cpp
> ===================================================================
> --- lib/ProfileData/SampleProfReader.cpp
> +++ lib/ProfileData/SampleProfReader.cpp
> @@ -356,18 +356,18 @@
>  /// \brief Prepare a memory buffer for the contents of \p Filename.
>  ///
>  /// \returns an error code indicating the status of the buffer.
> -static std::error_code
> -setupMemoryBuffer(std::string Filename, std::unique_ptr<MemoryBuffer> &Buffer) {
> +static ErrorOr<std::unique_ptr<MemoryBuffer>>
> +setupMemoryBuffer(std::string Filename) {
>    auto BufferOrErr = MemoryBuffer::getFileOrSTDIN(Filename);
>    if (std::error_code EC = BufferOrErr.getError())
>      return EC;
> -  Buffer = std::move(BufferOrErr.get());
> +  auto Buffer = std::move(BufferOrErr.get());
>  
>    // Sanity check the file.
>    if (Buffer->getBufferSize() > std::numeric_limits<unsigned>::max())
>      return sampleprof_error::too_large;
>  
> -  return sampleprof_error::success;
> +  return std::move(Buffer);
>  }
>  
>  /// \brief Create a sample profile reader based on the format of the input file.
> @@ -379,18 +379,21 @@
>  /// \param C The LLVM context to use to emit diagnostics.
>  ///
>  /// \returns an error code indicating the status of the created reader.
> -std::error_code
> -SampleProfileReader::create(StringRef Filename,
> -                            std::unique_ptr<SampleProfileReader> &Reader,
> -                            LLVMContext &C) {
> -  std::unique_ptr<MemoryBuffer> Buffer;
> -  if (std::error_code EC = setupMemoryBuffer(Filename, Buffer))
> +ErrorOr<std::unique_ptr<SampleProfileReader>>
> +SampleProfileReader::create(StringRef Filename, LLVMContext &C) {
> +  auto BufferOrError = setupMemoryBuffer(Filename);
> +  if (std::error_code EC = BufferOrError.getError())
>      return EC;
>  
> +  auto Buffer = std::move(BufferOrError.get());
> +  std::unique_ptr<SampleProfileReader> Reader;
>    if (SampleProfileReaderBinary::hasFormat(*Buffer))
>      Reader.reset(new SampleProfileReaderBinary(std::move(Buffer), C));
>    else
>      Reader.reset(new SampleProfileReaderText(std::move(Buffer), C));
>  
> -  return Reader->readHeader();
> +  if (std::error_code EC = Reader->readHeader())
> +    return EC;
> +
> +  return std::move(Reader);
>  }
> Index: lib/ProfileData/SampleProfWriter.cpp
> ===================================================================
> --- lib/ProfileData/SampleProfWriter.cpp
> +++ lib/ProfileData/SampleProfWriter.cpp
> @@ -107,18 +107,20 @@
>  /// \param Format Encoding format for the profile file.
>  ///
>  /// \returns an error code indicating the status of the created writer.
> -std::error_code
> -SampleProfileWriter::create(StringRef Filename,
> -                            std::unique_ptr<SampleProfileWriter> &Writer,
> -                            SampleProfileFormat Format) {
> +ErrorOr<std::unique_ptr<SampleProfileWriter>>
> +SampleProfileWriter::create(StringRef Filename, SampleProfileFormat Format) {
>    std::error_code EC;
> +  std::unique_ptr<SampleProfileWriter> Writer;
>  
>    if (Format == SPF_Binary)
>      Writer.reset(new SampleProfileWriterBinary(Filename, EC));
>    else if (Format == SPF_Text)
>      Writer.reset(new SampleProfileWriterText(Filename, EC));
>    else
>      EC = sampleprof_error::unrecognized_format;
>  
> -  return EC;
> +  if (EC)
> +    return EC;
> +
> +  return std::move(Writer);
>  }
> Index: lib/Transforms/Scalar/SampleProfile.cpp
> ===================================================================
> --- lib/Transforms/Scalar/SampleProfile.cpp
> +++ lib/Transforms/Scalar/SampleProfile.cpp
> @@ -737,12 +737,13 @@
>                      "Sample Profile loader", false, false)
>  
>  bool SampleProfileLoader::doInitialization(Module &M) {
> -  if (std::error_code EC =
> -          SampleProfileReader::create(Filename, Reader, M.getContext())) {
> +  auto ReaderOrErr = SampleProfileReader::create(Filename, M.getContext());
> +  if (std::error_code EC = ReaderOrErr.getError()) {
>      std::string Msg = "Could not open profile: " + EC.message();
>      M.getContext().diagnose(DiagnosticInfoSampleProfile(Filename.data(), Msg));
>      return false;
>    }
> +  Reader = std::move(ReaderOrErr.get());
>    ProfileIsValid = (Reader->read() == sampleprof_error::success);
>    return true;
>  }
> Index: tools/llvm-profdata/llvm-profdata.cpp
> ===================================================================
> --- tools/llvm-profdata/llvm-profdata.cpp
> +++ tools/llvm-profdata/llvm-profdata.cpp
> @@ -49,10 +49,11 @@
>  
>    InstrProfWriter Writer;
>    for (const auto &Filename : Inputs) {
> -    std::unique_ptr<InstrProfReader> Reader;
> -    if (std::error_code ec = InstrProfReader::create(Filename, Reader))
> +    auto ReaderOrErr = InstrProfReader::create(Filename);
> +    if (std::error_code ec = ReaderOrErr.getError())
>        exitWithError(ec.message(), Filename);
>  
> +    auto Reader = std::move(ReaderOrErr.get());
>      for (const auto &I : *Reader)
>        if (std::error_code EC =
>                Writer.addFunctionCounts(I.Name, I.Hash, I.Counts))
> @@ -66,18 +67,19 @@
>  void mergeSampleProfile(cl::list<std::string> Inputs, StringRef OutputFilename,
>                          sampleprof::SampleProfileFormat OutputFormat) {
>    using namespace sampleprof;
> -  std::unique_ptr<SampleProfileWriter> Writer;
> -  if (std::error_code EC = SampleProfileWriter::create(OutputFilename.data(),
> -                                                       Writer, OutputFormat))
> +  auto WriterOrErr = SampleProfileWriter::create(OutputFilename, OutputFormat);
> +  if (std::error_code EC = WriterOrErr.getError())
>      exitWithError(EC.message(), OutputFilename);
>  
> +  auto Writer = std::move(WriterOrErr.get());
>    StringMap<FunctionSamples> ProfileMap;
>    for (const auto &Filename : Inputs) {
> -    std::unique_ptr<SampleProfileReader> Reader;
> -    if (std::error_code EC =
> -            SampleProfileReader::create(Filename, Reader, getGlobalContext()))
> +    auto ReaderOrErr =
> +        SampleProfileReader::create(Filename, getGlobalContext());
> +    if (std::error_code EC = ReaderOrErr.getError())
>        exitWithError(EC.message(), Filename);
>  
> +    auto Reader = std::move(ReaderOrErr.get());
>      if (std::error_code EC = Reader->read())
>        exitWithError(EC.message(), Filename);
>  
> @@ -129,10 +131,11 @@
>  int showInstrProfile(std::string Filename, bool ShowCounts,
>                       bool ShowAllFunctions, std::string ShowFunction,
>                       raw_fd_ostream &OS) {
> -  std::unique_ptr<InstrProfReader> Reader;
> -  if (std::error_code EC = InstrProfReader::create(Filename, Reader))
> +  auto ReaderOrErr = InstrProfReader::create(Filename);
> +  if (std::error_code EC = ReaderOrErr.getError())
>      exitWithError(EC.message(), Filename);
>  
> +  auto Reader = std::move(ReaderOrErr.get());
>    uint64_t MaxFunctionCount = 0, MaxBlockCount = 0;
>    size_t ShownFunctions = 0, TotalFunctions = 0;
>    for (const auto &Func : *Reader) {
> @@ -182,11 +185,11 @@
>                        bool ShowAllFunctions, std::string ShowFunction,
>                        raw_fd_ostream &OS) {
>    using namespace sampleprof;
> -  std::unique_ptr<SampleProfileReader> Reader;
> -  if (std::error_code EC =
> -          SampleProfileReader::create(Filename, Reader, getGlobalContext()))
> +  auto ReaderOrErr = SampleProfileReader::create(Filename, getGlobalContext());
> +  if (std::error_code EC = ReaderOrErr.getError())
>      exitWithError(EC.message(), Filename);
>  
> +  auto Reader = std::move(ReaderOrErr.get());
>    Reader->read();
>    if (ShowAllFunctions || ShowFunction.empty())
>      Reader->dump(OS);