[PATCH] D53517: [FileCheck] Parse command-line options from FILECHECK_OPTS

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added inline comments.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:28
+Options are parsed from the command line and from the environment variable
+``FILECHECK_OPTS``.
+
----------------
probinson wrote:
> This currently implies the command line is done first, which it isn't.
> "Options are parsed from the env var and then from the command line" or something along those lines.
Good point.  I'll just reverse that.  Thanks.

Either way, as you hinted, the order is only implied here.  I'm intentionally avoiding being explicit because I know of no FileCheck option where the order of multiple occurrences //should// matter.   The only option I know of for which order //does// matter is -D: all definitions of a variable after the first are ignored.  My guess is that behavior is unintentional and multiply defined variables should be errors instead.  I just wrote a quick patch to make them errors, and I see no new test fails from check-all.  I'm happy to submit that patch... or to be advised that I need to rethink this issue.

On the other hand, for the (internal) documentation of ParseCommandLineOptions, I was explicit about the order because ParseCommandLineOptions might one day be reused by some other program where this issue needs to be considered.


================
Comment at: llvm/utils/FileCheck/FileCheck.cpp:117
   InitLLVM X(argc, argv);
-  cl::ParseCommandLineOptions(argc, argv);
+  cl::ParseCommandLineOptions(argc, argv, "", nullptr, "FILECHECK_OPTS");
 
----------------
probinson wrote:
> For arguments such as "" and nullptr, generally the style is to put the parameter name in a comment so the reader knows what is being omitted.  So in this case something like:
> ```
> ..., /*Overview*/ "", /*Errs*/ nullptr, "FILECHECK_OPTS");
> ```
> 
Thanks. Just out of curiosity, is this style documented somewhere?  I'll make the change regardless.


https://reviews.llvm.org/D53517