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

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60382