[PATCH] D36201: Merge manifest namespaces.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:106-107
+  for (auto &Arg : InputArgs) {
+    if (!(Arg->getOption().matches(OPT_unsupported) ||
+          Arg->getOption().matches(OPT_supported))) {
+      reportError(Twine("invalid option ") + Arg->getSpelling());
----------------
ecbeckmann wrote:
> ruiu wrote:
> > `!A && !B` is preferred over `!(A || B)`.
> > 
> > But do you have to do this way? I think usually we handle OPT_UNKNOWN to report unknown options instead of putting all known options to some groups. Look at https://github.com/llvm-project/llvm-project-20170507/blob/dd24779ef3e3e592cc0c7561a2fb1fbe3309fa1c/lld/ELF/DriverUtils.cpp#L112 for example.
> I have tried this....however for some reason OPT_UNKNOWN never filters any arguments for me.  It seems for some reason the invalid flags are recognized as inputs, not flags.  I'm not sure if I'm setting up the options incorrectly or if this is a bug in the option library.....also not sure if this is worth the effort to fix.
Ah, okay, I think I know why it behaves like that. Unlike Unix, Windows command line options start with '/', and the character is ambiguous as to whether it is a path (e.g. /bin/ls) or an option (e.g. /manifest). LLVM's option parser's behavior is that, if an option "/foo" is defined, "/foo" is handled as an option. Otherwise, "/foo" is handled as a path.

This is an unavoidable consequence if you allow both Unix-style paths and Windows-style command line options. Since that's what we want, it is okay.

I think `Group<supported>` does not do anything. I believe your llvm-mt command still does not warn on unknown options. Please try `llvm-mt /foo`, for example. My guess is that it will warn that it cannot find file "/foo". You may want to just remove `Group<supported>` from Options.td and also remove this piece of new code.


https://reviews.llvm.org/D36201