Add format_provider for the Error class

[labath]

Summary

The formatter supports the same options as the string-like classes, i.e. the
ability to truncate the displayed string. I don't anticipate it would be much
used, but it seems consistent.

[clayborg]

See inlined comment.

include/lldb/Core/Error.h

308–309

Do we want to prefix this output with "error: " followed by the error string?

[zturner]

include/lldb/Core/Error.h

308–309

If we prefix it by something, then perhaps we should incldue the error category as well. Is it a Windows error, a generic error, a posix error?

(Note that in the future I still would like to kill lldb::Error and replace it with llvm::Error which is much safer / more extensible, but no objection to this kind of change going in in the interim.)

[labath]

I wouldn't put just "error" as it is quite superfluous ("No such file." sounds like an error even without that prefix). And then you don't have the option to remove it if you need it for some reason (unless you add more format options...). I'd keep it without it, as it is easy to add it manually if you need it.

Adding the error category sounds like a better idea. I am fine without it, but I can add it if you want to.

If we're going to be changing stuff, I'd consider changing the success case to print "Success". (But I'd also change the AsCString method to keep things consistent).

[labath]

I think we should keep the AsCString and the formatted output identical, to minimize surprises. So if we want to add category, I think it should go into the AsCString as well.

[clayborg]

Lets start with this. I would by default just emit the error string and then let users opt in via additional format modifiers to show the "error: " prefix and add the category.

[clayborg]

You can't add anything extra to the AsCString() since it returns a "const char *". You can't return a StringRef because it isn't backed by anything. You could return a std::string.

My vote would be to leave AsCString() alone and have it just return a pointer to the string buffer that it owns, and let the formatv stuff do the extra formatting.

[zturner]

Technically it's backed by the error instance, so you could return a StringRef as long as you said that its lifetime is tied to the lifetime of the Error instance. Not super unreasonable, but also not high priority. I would vote for getting rid of c strings wherever possible, but in this case I think a std::string would be better. On the other hand, as mentioned it's kind of orthogonal to this change, so for now I wouldn't worry about it at all and just leave AsCString alone as Greg said.

[labath]

In D28519#641938, @clayborg wrote:

You can't add anything extra to the AsCString() since it returns a "const char *". You can't return a StringRef because it isn't backed by anything. You could return a std::string.

My vote would be to leave AsCString() alone and have it just return a pointer to the string buffer that it owns, and let the formatv stuff do the extra formatting.

The way I would add it is to add whatever text we want directly to the m_string variable that backs the returned const char *. Basically, I believe that if a function has a "natural" string conversion function (which AsCString is) then the default formatv conversion should be just that (obviously the formatv conversion can do other fancy stuff with the format modifiers, but that's another story).

I believe we have converged to keeping this patch as is. If I don't hear any comments, I am going to land this tomorrow.

[clayborg]

Looks fine.

include/lldb/Core/Error.h

 //===-- Error.h -------------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef __DCError_h__
 #define __DCError_h__
 #if defined(__cplusplus)
 
 #include "llvm/Support/DataTypes.h"
+#include "llvm/Support/FormatVariadic.h"
 
 #include <cstdarg>
 #include <cstdio>
 #include <string>
 
 #include "lldb/lldb-private.h"
 
 #include "llvm/Support/FormatVariadic.h"
protected:
   //------------------------------------------------------------------
   ValueType m_code;             ///< Error code as an integer value.
   lldb::ErrorType m_type;       ///< The type of the above error code.
   mutable std::string m_string; ///< A string representation of the error code.
 };
 
 } // namespace lldb_private
 
+namespace llvm {
+template <> struct llvm::format_provider<lldb_private::Error> {
+  static void format(const lldb_private::Error &error, llvm::raw_ostream &OS,
+                     llvm::StringRef Options) {
+    llvm::format_provider<llvm::StringRef>::format(error.AsCString(), OS,
+                                                   Options);
+  }
+};
+}
+
 #endif // #if defined(__cplusplus)
 #endif // #ifndef __DCError_h__

unittests/Core/CMakeLists.txt

 add_lldb_unittest(LLDBCoreTests
   ArchSpecTest.cpp
   BroadcasterTest.cpp
   DataExtractorTest.cpp
+  ErrorTest.cpp
   ListenerTest.cpp
   ScalarTest.cpp
   StructuredDataTest.cpp
   TimerTest.cpp
   )

unittests/Core/ErrorTest.cpp

+//===-- ErrorTest.cpp -------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "gtest/gtest.h"
+#include "lldb/Core/Error.h"
+
+using namespace lldb_private;
+
+TEST(ErrorTest, Formatv) {
+  EXPECT_EQ("", llvm::formatv("{0}", Error()).str());
+  EXPECT_EQ("Hello Error", llvm::formatv("{0}", Error("Hello Error")).str());
+  EXPECT_EQ("Hello", llvm::formatv("{0:5}", Error("Hello Error")).str());
+}