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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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(
----------------
jhenderson wrote:
> 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.
OK, I've used `llvm_unreachable`.


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

https://reviews.llvm.org/D86772