[PATCH] D62753: [llvm-lipo] Implement -archs

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:154
+
+  llvm_unreachable("llvm-lipo action unspecified");
 }
----------------
smeenai wrote:
> alexshap wrote:
> > smeenai wrote:
> > > compnerd wrote:
> > > > I don't think that this should be unreachable - if the action is unspecified, that can occur normally in the way that the user invokes the tool.  Perhaps this should be a proper error?
> > > Lines 123 through 133 should ensure we have a single action argument of the expected type. My switch statement suggestion above should make that structure clearer.
> > llvm_unreachable is for internal logical errors, here if the specified command line arguments are incorrect we should make use of reportError, the tool should not crash.
> I actually think this would be an internal logic error:
> 
> * If the user hasn't specified exactly one action argument, we'll have errored out already.
> * The only action arguments we currently support are `-verify_arch` and `-archs`.
> * Therefore, if our option isn't one of them, something is pretty wrong with our program logic.
> 
> I'm okay with using `reportError` for this though.
imo it's not. llvm_unreachable wouldn't be the right choice here for the following reason: for example, if a user has misspelled a flag or option the tool should not crash with assert/llvm_unreachable, it should exit normally (report error and exit with non-zero code)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62753