[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:
> > 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.
> I think you're right.  It also seems to be somewhat of a bug with the option parsing library, OPT_UNKNOWN never seems to do anything.  I just tried with lld-link and it seems unable to detect invalid flags, complaining rather that it can't open the given file.  So that line in DriverUtils.cpp doesn't actually do anything.
> 
> My Group<supported> example does work however.  For example, running llvm-mt /foo does indeed detect the invalid option given, instead of interpreting it as an input file.
OPT_UNKNOWN does work if you pass an argument with `-` prefix. Try `-foo` for example. I think it'll be handled as an OPT_UNKNOWN option.

I believe that your Group<supported> doesn't really work. Well, it's working in some sense, but this is the same as iterating over OPT_INPUT, no? I mean, I believe your code is the same as

  for (auto *Arg : Args.filtered(OPT_INPUT))
    reportError(Twine("invalid option ") + Arg->getSpelling());


https://reviews.llvm.org/D36201