[PATCH] D60382: FileCheck [2/12]: Stricter parsing of -D option

Mon Apr 15 03:41:44 PDT 2019

thopre added a comment.

In D60382#1464553 <https://reviews.llvm.org/D60382#1464553>, @probinson wrote:

> In D60382#1464412 <https://reviews.llvm.org/D60382#1464412>, @thopre wrote:
>
> > In D60382#1464197 <https://reviews.llvm.org/D60382#1464197>, @probinson wrote:
> >
> > > Wouldn't it be preferable to put the command-line option validation in the driver, rather than here?
> > >  The division of responsibilities between the driver and the library is really not clear-cut; normally I'd expect the driver to do option parsing/validation and the library to simply accept the list.  (I understand that FileCheck currently isn't always organized that way, but it's something I'd like to see happen.)
> >
> >
> > If the library was only used by the FileCheck executable I would agree but since it is used as a library by some of the tests I believe it makes sense to test here. Some of the code could go away if the library was receiving global defines as a vector of pair of StringRef but the validation of variable name would still be necessary IMO.
>
>
> IMO this is not a user-facing API.  I think it's entirely reasonable to trust the unittests to call the API correctly, and have the user-facing driver validate the user-provided input.  If you want extra checks in the library in Asserts mode, that would be fine.

Actually, while this parsing code can easily be moved to the FileCheck executable by assuming the name is well formed, follow-on commits eventually allow to define a numeric variable from the command-line to a numeric expression so the parsing code is reused to avoid a lot of code duplication and the parsing code expect a SourceMgr to print the diagnostic to.

Therefore, if we want to remove the diagnostic from that function we need to remove the ability to define a numeric variable from a numeric expression (which allows stuff like -D#NUMVAR1=10 -D#NUMVAR2=NUMVAR1+4. How useful do you think this ability is?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60382