[Support] Introduce createStringError helper function

[vleschuk]

Summary

The function in question is copy-pasted lots of times in DWARF-related classes. Thus it will make sense to place its implementation into the Support library.

[JDevlieghere]

Can you add a test for the case where no Vals are provided? I remember something about that requiring an overload without the variadic template arguments.

[vleschuk]

In D49824#1176004, @JDevlieghere wrote:

Can you add a test for the case where no Vals are provided? I remember something about that requiring an overload without the variadic template arguments.

Do you mean something like that the following?

logAllUnhandledErrors(createStringError("bar"), S, "");                         
EXPECT_EQ(S.str(), "bar\n") ;

I can add it but it will result in compiler warnings "warning: format string is not a string literal (potentially insecure) [-Wformat-security]" which is not looking good. We can temporary disable warnings for this test using compiler-specific pragmas but it will be very messy...

[lhames]

include/llvm/Support/Error.h

1125–1133 ↗

(On Diff #157382)

For now I think this needs to take the std::error_code for the StringError as an argument too, rather than using inconvertibleErrorCode().

The reason for this is that using inconvertibleErrorCode breaks Error / std::error_code interoperability: any attempt to convert an Error value constructed with an inconvertibleErrorCode triggers an immediate abort. It is meant to be used explicitly, and only for code that explicitly documents that it does not support interoperability. It should not be default behavior.

Given the increasingly wide adoption of Error we could talk about changing this policy, but that is a discussion we should have on the dev list where it will reach a wide audience.

[vleschuk]

Added explicit std::error_code param to createStringError() function.

[lhames]

In D49824#1176015, @vleschuk wrote:

In D49824#1176004, @JDevlieghere wrote:

Can you add a test for the case where no Vals are provided? I remember something about that requiring an overload without the variadic template arguments.

Do you mean something like that the following?

logAllUnhandledErrors(createStringError("bar"), S, "");                         
EXPECT_EQ(S.str(), "bar\n") ;

I can add it but it will result in compiler warnings "warning: format string is not a string literal (potentially insecure) [-Wformat-security]" which is not looking good. We can temporary disable warnings for this test using compiler-specific pragmas but it will be very messy...

You can add an overload for the degenerate case to fix this:

Error createStringError(std::error_code EC, char const *Msg) {
  return make_error<StringError>(Msg, EC);
}

[vleschuk]

Added overloaded version of createStringError() function which is not variadic, also added corresponding test.

[lhames]

Cool. Looks good to me. Thanks Victor!

[JDevlieghere]

Lang's suggestion was indeed what I had in mind. Thanks!

llvm/trunk/include/llvm/Support/Error.h

 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Config/abi-breaking.h"
 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <cstdlib>
 #include <functional>
 #include <memory>
 #include <new>
public:
 
   const std::string &getMessage() const { return Msg; }
 
 private:
   std::string Msg;
   std::error_code EC;
 };
 
+/// Create formatted StringError object.
+template <typename... Ts>
+Error createStringError(std::error_code EC, char const *Fmt,
+                        const Ts &... Vals) {
+  std::string Buffer;
+  raw_string_ostream Stream(Buffer);
+  Stream << format(Fmt, Vals...);
+  return make_error<StringError>(Stream.str(), EC);
+}
+
+Error createStringError(std::error_code EC, char const *Msg);
+
 /// Helper for check-and-exit error handling.
 ///
 /// For tool use only. NOT FOR USE IN LIBRARY CODE.
 ///
 class ExitOnError {
 public:
   /// Create an error on exit helper.
   ExitOnError(std::string Banner = "", int DefaultErrorExitCode = 1)

llvm/trunk/lib/Support/Error.cpp

StringError::StringError(const Twine &S, std::error_code EC)
     : Msg(S.str()), EC(EC) {}
 
 void StringError::log(raw_ostream &OS) const { OS << Msg; }
 
 std::error_code StringError::convertToErrorCode() const {
   return EC;
 }
 
+Error createStringError(std::error_code EC, char const *Msg) {
+  return make_error<StringError>(Msg, EC);
+}
+
 void report_fatal_error(Error Err, bool GenCrashDiag) {
   assert(Err && "report_fatal_error called with success value");
   std::string ErrMsg;
   {
     raw_string_ostream ErrStream(ErrMsg);
     logAllUnhandledErrors(std::move(Err), ErrStream, "");
   }
   report_fatal_error(ErrMsg);

llvm/trunk/unittests/Support/ErrorTest.cpp

TEST(Error, StringError) {
   EXPECT_EQ(S.str(), "foo42\n") << "Unexpected StringError log result";
 
   auto EC =
     errorToErrorCode(make_error<StringError>("", errc::invalid_argument));
   EXPECT_EQ(EC, errc::invalid_argument)
     << "Failed to convert StringError to error_code.";
 }
 
+TEST(Error, createStringError) {
+  static const char *Bar = "bar";
+  static const std::error_code EC = errc::invalid_argument;
+  std::string Msg;
+  raw_string_ostream S(Msg);
+  logAllUnhandledErrors(createStringError(EC, "foo%s%d0x%" PRIx8, Bar, 1, 0xff),
+                        S, "");
+  EXPECT_EQ(S.str(), "foobar10xff\n")
+    << "Unexpected createStringError() log result";
+
+  S.flush();
+  Msg.clear();
+  logAllUnhandledErrors(createStringError(EC, Bar), S, "");
+  EXPECT_EQ(S.str(), "bar\n")
+    << "Unexpected createStringError() (overloaded) log result";
+
+  S.flush();
+  Msg.clear();
+  auto Res = errorToErrorCode(createStringError(EC, "foo%s", Bar));
+  EXPECT_EQ(Res, EC)
+    << "Failed to convert createStringError() result to error_code.";
+}
+
 // Test that the ExitOnError utility works as expected.
 TEST(Error, ExitOnError) {
   ExitOnError ExitOnErr;
   ExitOnErr.setBanner("Error in tool:");
   ExitOnErr.setExitCodeMapper([](const Error &E) {
     if (E.isA<CustomSubError>())
       return 2;
     return 1;