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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:154
+
+  llvm_unreachable("llvm-lipo action unspecified");
 }
----------------
alexshap wrote:
> 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)
> 
I agree, but if the user had misspelled a flag or option, we would have also handled it already (we check for unknown arguments explicitly above).

Basically, in order to have gotten to this switch statement, we should have performed all our input validation already. If we're ending up with an unknown option here, that means something is wrong with our input validation logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62753