[PATCH] D86772: [llvm-readobj] - Remove Error.cpp,.h and drop dependencies in the code.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:582
+
+    Binary* Bin = ChildOrErr->get();
+    if (ObjectFile *Obj = dyn_cast<ObjectFile>(Bin))
----------------
This doesn't look clang-formatted.


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:589-590
+      reportWarning(
+          createStringError(errc::invalid_argument,
+                            "unrecognized file type of " + Bin->getFileName()),
+          Arc->getFileName());
----------------
I think the message should be: "foo.o has an unrecognized file type" (or possibly even "unsupported" instead of "unrecognized").


================
Comment at: llvm/tools/llvm-readobj/llvm-readobj.cpp:640
   else
-    reportError(errorCodeToError(readobj_error::unrecognized_file_format),
-                File);
+    // Not tested, because all possible file types are handled.
+    reportError(
----------------
grimar wrote:
> sbc100 wrote:
> > Should we assert or llvm_unreachable here then?  Since it should be impossible to get here because the above "case" statement should handle all valid file types?
> I don't think that it is right to use `llvm_unreachable`, because I guess it might be possible that one day a new object format appears, but will be unsupported by `llvm-readelf`. We probably don't want to fail in this case, but want to show an error message I think.
> 
> Regarding adding an `assert`, I don't know. I don't have any stong feeling about it.
I think `llvm_unreachable` is the right thing here. Currently, it should be impossible to reach this code (hence "unreachable"). The only way that it would be reachable is if somebody adds a new variety of `Binary` without properly adding support for it everywhere. This is a programmer error, so `assert`/`llvm_unreachable` is the way to report this. Since this code can't be executed under current state, `llvm_unreachable` is more appropriate than `assert` (which is better used for checking pre/post conditions).

If one day a user does add a new file type, which shouldn't be supported by llvm-readobj, they can change to reporting a normal error as you are currently doing. This would be a conscious decision on their part, to not (at that point) add the support.


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

https://reviews.llvm.org/D86772