[PATCH] D66286: [llvm-readobj/llvm-readelf] - Improve/cleanup the error reporting API.

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: tools/llvm-readobj/ELFDumper.cpp:141-144
+      // TODO: Add a section index and the file name to this warning.
+      reportWarning(createError("invalid section size (" + Twine(Size) +
+                                ") or entity size (" + Twine(EntSize) + ")"),
+                    "<?>");
----------------
I think `<?>` is confusing, and is worth fixing in this patch instead of deferring to a followup cleanup.

Could you perhaps make `DynRegionInfo` take a required filename parameter which can be initialized in `ELFDumper`'s constructor so it's always available?


================
Comment at: tools/llvm-readobj/llvm-readobj.cpp:400
+                  [&](const ErrorInfoBase &EI) {
+                    fouts().flush();
+                    errs() << "\n";
----------------
... any idea why this was necessary before? This is a different stream. Might be nice to have a comment in the code. (Same for above in `error()`).


================
Comment at: tools/llvm-readobj/llvm-readobj.h:24-25
   // Various helper functions.
-  LLVM_ATTRIBUTE_NORETURN void reportError(Twine Msg);
   void reportError(Error Err, StringRef Input); 
-  void reportWarning(Twine Msg);
-  void reportWarning(StringRef Input, Error Err);
-  void warn(llvm::Error Err);
-  void error(std::error_code EC);
+  void reportError(std::error_code EC, StringRef Input);
+  void reportWarning(Error Err, StringRef Input);
----------------
Can these both be `LLVM_ATTRIBUTE_NORETURN`? I think they should have been so before too.


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

https://reviews.llvm.org/D66286