[PATCH] D59946: [llvm-readobj] Improve error message for for --string-dump

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer marked 2 inline comments as done.
StephenTozer added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ObjDumper.cpp:58-59
   }
-  return make_error<StringError>("invalid section reference",
-                                 object::object_error::parse_failed);
+  return make_error<StringError>(
+      formatv("could not find section '{0}'", SecName),
+      object::object_error::parse_failed);
----------------
jhenderson wrote:
> I think you'd be better off using createStringError() here, which takes variadic arguments and formats in printf style.
The SecName variable in this case is a StringRef, which doesn't work as nicely with printf-style formatting: it has an internal char*, but this is not necessarily null terminated, so would need to be converted to an intermediate string variable to be compatible. Since it has built-in formatting with formatv, I thought that the more appropriate choice.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:405-406
 
-static void reportError(StringRef Input, Error Err) {
-  if (Input == "-")
-    Input = "<stdin>";
-  std::string ErrMsg;
-  {
-    raw_string_ostream ErrStream(ErrMsg);
-    logAllUnhandledErrors(std::move(Err), ErrStream, Input + ": ");
-  }
-  reportError(ErrMsg);
+static void reportError(StringRef Input, std::error_code EC) {
+  reportError(Input, errorCodeToError(EC));
 }
----------------
jhenderson wrote:
> Nit: Could you move this above the reportError immediately above, so that it lines up well in diffs with the old version, please?
The move is messy, but since this function now calls the other reportError function it has to be below it. The two alternatives to changing the function order would be to declare the above function earlier in this .cpp file, or declare it in the .h file. The latter seems like a bad idea to me, since it doesn't need to be made available outside of this file. I thought inserting an early declaration for one of the 3 overloads in the .cpp file was messier than changing the order, but if preserving the diff is more important then I can do that instead.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59946/new/

https://reviews.llvm.org/D59946