[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:
> > smeenai wrote:
> > > 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.
> > Basically, under which circumstances could the `default` case be reached? I believe that with our current code we should never be able to reach it.
> depending on how good our validation will be in the future, this invariant ("unreachable") might be preserved, if one day this invariant gets violated - okay, the tool will start crashing on some incorrectly specified flags. So this was more like a general consideration what the tool should do if a flag or a combination of flags has been specified incorrectly, in particular, it should not crash. 
Sounds good, I can live with that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62753