[Support] Introduce formatv variant of createStringError

[bulbazord]

Many times I have found myself wanting to create a StringError with the ability to interpolate a StringRef into the error string. This can be achieved with:

StringRef Foo("...");
auto Err = createStringError(..., "Something went wrong: %s", Foo.str().c_str());

However, this requires us to construct a temporary std::string (which may perform a memory allocation if large enough).

I propose a new variant of createStringError called createStringErrorV which uses formatv under the hood. This allows the above example to become:

StringRef Foo("...");
auto Err = createStringErrorV(..., "Something went wrong: {0}", Foo);

[llvmbot]

@llvm/pr-subscribers-llvm-support

Author: Alex Langford (bulbazord)

Changes

Many times I have found myself wanting to create a StringError with the ability to interpolate a StringRef into the error string. This can be achieved with:

StringRef Foo("...");
auto Err = createStringError(..., "Something went wrong: %s", Foo.str().c_str());

However, this requires us to construct a temporary std::string (which may perform a memory allocation if large enough).

I propose a new variant of createStringError called createStringErrorv which uses formatv under the hood. This allows the above example to become:

StringRef Foo("...");
auto Err = createStringError(..., "Something went wrong: {0}", Foo);

---

Full diff: https://github.com/llvm/llvm-project/pull/80493.diff

2 Files Affected:

• (modified) llvm/include/llvm/Support/Error.h (+10)
• (modified) llvm/unittests/Support/ErrorTest.cpp (+17)

diff --git a/llvm/include/llvm/Support/Error.h b/llvm/include/llvm/Support/Error.h
index bb4f38f7ec355..e13543d1681c0 100644
--- a/llvm/include/llvm/Support/Error.h
+++ b/llvm/include/llvm/Support/Error.h
@@ -22,6 +22,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/Format.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <cstdint>
@@ -1261,6 +1262,15 @@ inline Error createStringError(std::errc EC, char const *Fmt,
   return createStringError(std::make_error_code(EC), Fmt, Vals...);
 }
 
+template <typename... Ts>
+inline Error createStringErrorv(std::error_code EC, const char *Fmt,
+                                const Ts &...Vals) {
+  std::string Buffer;
+  raw_string_ostream Stream(Buffer);
+  Stream << formatv(Fmt, Vals...);
+  return make_error<StringError>(Stream.str(), EC);
+}
+
 /// This class wraps a filename and another Error.
 ///
 /// In some cases, an error needs to live along a 'source' name, in order to
diff --git a/llvm/unittests/Support/ErrorTest.cpp b/llvm/unittests/Support/ErrorTest.cpp
index 11f93203597bf..17de7aaf4c9c1 100644
--- a/llvm/unittests/Support/ErrorTest.cpp
+++ b/llvm/unittests/Support/ErrorTest.cpp
@@ -472,6 +472,23 @@ TEST(Error, createStringError) {
     << "Failed to convert createStringError() result to error_code.";
 }
 
+TEST(Error, createStringErrorv) {
+  static llvm::StringRef Bar("bar");
+  static const std::error_code EC = errc::invalid_argument;
+  std::string Msg;
+  raw_string_ostream S(Msg);
+  logAllUnhandledErrors(createStringErrorv(EC, "foo{0}{1}{2:x}", Bar, 1, 0xff),
+                        S);
+  EXPECT_EQ(S.str(), "foobar10xff\n")
+      << "Unexpected createStringErrorv() log result";
+
+  S.flush();
+  Msg.clear();
+  auto Res = errorToErrorCode(createStringErrorv(EC, "foo{0}", Bar));
+  EXPECT_EQ(Res, EC)
+      << "Failed to convert createStringErrorv() result to error_code.";
+}
+
 // Test that the ExitOnError utility works as expected.
 TEST(ErrorDeathTest, ExitOnError) {
   ExitOnError ExitOnErr;

[topperc]

I don't know what all your use cases are but for that specific example you can write createStringError(..., "Something went wrong: " + Foo); which uses the Twine version.

[MaskRay]

Thanks for the PR. This helps avoid the dangerous llvm::format.

I left a comment on https://discourse.llvm.org/t/llvm-format-is-dangerous/71994/16?u=maskray so that more folks are ware of this PR. This LGTM but it should wait a bit seeking more opinions.

[kuhar]

nit: I like the idea but it's really hard for me to tell this apart from the old function -- the only difference is the trailing lowercase v at the end. Could we either pick a different name or capitalize it consistently with the function (V)?

[bulbazord]

I'd be fine with capitalizing the v at the end. I'm also alright with createStringErrorVariadic as well. If anyone feels particularly strongly about either of these, let me know. Otherwise I will likely choose the former.

[bulbazord]

I don't know what all your use cases are but for that specific example you can write createStringError(..., "Something went wrong: " + Foo); which uses the Twine version.

Yep, that's an option for this example. I was also thinking about string interpolation with other types (including some from LLDB directly). In LLDB we often construct StringErrors with multiple bits of information in them, not just strings. If we have a std::string or a const char * then this isn't a concern, but if we have a StringRef, we either need to do the .str().c_str() song and dance or build our message with formatv first. The former means potential (otherwise unneeded) small allocations, the second is a lot of code compared to a variation of createStringError.

Thanks for the PR. This helps avoid the dangerous llvm::format.

I left a comment on https://discourse.llvm.org/t/llvm-format-is-dangerous/71994/16?u=maskray so that more folks are ware of this PR. This LGTM but it should wait a bit seeking more opinions.

Thanks for doing that, I wasn't aware of that thread. I'll wait at least a few days before landing this to give folks the time to look it over and discuss.

[bulbazord]

Just updated this PR. If there are no objections, I'd like to land this by Friday.

[nikic]

This commit increases time to compile clang by 0.5% by adding an expensive include to a core header. The impact is large enough that we should make an effort to avoid it -- if all else fails, by putting this into a separate header.

[bulbazord]

This commit increases time to compile clang by 0.5% by adding an expensive include to a core header. The impact is large enough that we should make an effort to avoid it -- if all else fails, by putting this into a separate header.

I've not yet introduced any uses of this, so reverting should be cheap to do. I'll revert for now and figure something else out later.

[dwblaikie]

@lhames as author/owner of llvm::Error - might be interested.