[PATCH] D60390: FileCheck [10/12]: Add support for signed numeric values

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D60390#2059448 <https://reviews.llvm.org/D60390#2059448>, @thopre wrote:

> In D60390#2056652 <https://reviews.llvm.org/D60390#2056652>, @jhenderson wrote:
>
> > LGTM, with nit.
> >
> > Re. TEST_P, the best suggestion I've got is look at the examples in DWARFDebugLineTest.cpp, where I've used a few different tricks to achieve the goal of code re-use. In particular, `AdjustAddressFixtureBase` related test cases might be interesting to you. In your case, I'd probably have a set of parameters each for failing and passing combinations, and for addition and subtraction, for a total of 4 combinations, if I'm not mistaken. You'd have thin fixtures for each, but they could probably share some code via a base class.
> >
> > That all being said, only make those changes if you think they are worthwhile. I'm happy enough with this as-is.
>
>
> So far, I'm getting:
>
>   llvm/unittests/Support/FileCheckTest.cpp | 413 +++++++++++++++++++++++++++++++++-------------------
>   1 file changed, 263 insertions(+), 150 deletions(-)
>   
>
> It's also quite a lot of code change, so even if that is acceptable because the code is deemed more readable, it's better to look at it as a separate patch and land the current code (with the suggested spacing fix)


I'll leave it up to you as to whether you want to. This is probably sufficient as is.



================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:422
+  // Test both negative values.
+  expectOperationValueResult(operator+, - 10, - 10, - 20);
+
----------------
thopre wrote:
> jhenderson wrote:
> > Here and in various other places, not sure you want the space between `-` and `10` (same for `- 20` and various cases below).
> I don't like it either but this comes from clang-format-diff.py. Should I go against it?
If clang-format-diff.py was trying to format it that way, I'd consider that a bug in clang-format, so should be reported as such. The `-` there is a unary operator, and should be formatted as such (without a space).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60390