[PATCH] D60385: FileCheck [5/12]: Introduce regular numeric variables

Thu May 9 10:09:18 PDT 2019

thopre added inline comments.

================
Comment at: llvm/lib/Support/FileCheck.cpp:27-51
+/// Return value of this numeric variable.
+llvm::Optional<uint64_t> FileCheckNumExprVar::getValue() const {
+  if (!Defined)
+    return llvm::None;
+  return Value;
+}
+
----------------
probinson wrote:
> arsenm wrote:
> > thopre wrote:
> > > thopre wrote:
> > > > arsenm wrote:
> > > > > This can go away if you just use Optional as the member
> > > > Sorry I don't follow. An Optional variable as the member of the binop? How does that help? I still have to check whether that variable has a value or not.
> > > Ping?
> > I'm confused because the code here doesn't match what I see in the Phabricator email anymore.
> > 
> > If you are separately tracking Defined and returning None or Value, this is the same as just returning an Optional member
> I think the comment was originally on FileCheckNumericVariable, which used to have a separate bool, but @thopre changed this to an Optional a while ago. So this is done.
As Paul mentioned, I've addressed your other comment regarding using llvm::Optional instead of having an uint64_t and a bool. I've also adapted functions accordingly and some of them became trivial. This one was not much change since I need to call EvalBinop when the value is defined so I still need an if and separate return depending on whether the Optional value is true or not. Marking the issue as done since it seems I addressed the issue you were raising.

================
Comment at: llvm/test/FileCheck/var-scope.txt:4-6
+RUN: FileCheck -D#LOCNUM=1 -D'#$GLOBNUM=1' -input-file %s %s
+RUN: FileCheck -D#LOCNUM=1 -D'#$GLOBNUM=1' -check-prefixes CHECK,GLOBAL -input-file %s %s
+RUN: FileCheck -D#LOCNUM=1 -D'#$GLOBNUM=1' -check-prefixes CHECK,LOCAL3 -input-file %s %s
----------------
jhenderson wrote:
> What's the purpose of having these 3 as separate RUN lines, rather than a single one with check-prefixes=CHECK,GLOBAL,LOCAL3?
None, inherited from the original regex-scope.txt file. I've combined them now.

================
Comment at: llvm/test/FileCheck/verbose.txt:135
 VV-NEXT: {{C}}HECK-NOT: {{[{][{]z[}][}]yx$}}
-VV-NEXT:   {{^                 \^$}}
+VV-NEXT:   {{^                  \^$}}
 VV-NEXT: verbose.txt:[[@LINE+13]]:1: note: found here
----------------
jhenderson wrote:
> Why has this changed?
As per the patch description, I've enabled --strict-whitespace for these tests to check that the location information is correct in the CHECK I was adding but this check was failing. I've checked that the number of space I'm changing it to matches the column information from the diagnostic (18 spaces, caret on the 19th column).

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:23
+  EXPECT_TRUE(Value);
+  EXPECT_EQ((uint64_t)42, *Value);
+  EXPECT_TRUE(FooVar.setValue((uint64_t)43));
----------------
jhenderson wrote:
> What's the purpose of the cast here and elsewhere in this test?
To avoid a warning about comparison between sined (42) and unsigned (*Value which is uint64_t) integers. Elsewhere I've suffixed the literal with U, I've now normalized and that approach (ie. 42U in this case)

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:42
+TEST_F(FileCheckTest, NumExprEvalUndef) {
+  auto FooVar = new FileCheckNumericVariable("FOO", 42);
+  auto NumExpr = new FileCheckNumExpr(doAdd, FooVar, 18);
----------------
jhenderson wrote:
> Why is this a pointer here?
My bad, must have been a copy/paste from somewhere else. FileCheckNumericVariable are referenced by a std::unique_ptr in one of the member of FileCheckPatternContext so need to be a pointer to avoid freeing something from the stack in some of the unit test.

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:43
+  auto FooVar = new FileCheckNumericVariable("FOO", 42);
+  auto NumExpr = new FileCheckNumExpr(doAdd, FooVar, 18);
+
----------------
jhenderson wrote:
> Why is this a pointer?
Likewise, bad copy/paste. Fixed.

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:352
+  EXPECT_TRUE(NumExprVal);
+  EXPECT_EQ(*NumExprVal, 18U);
   EXPECT_TRUE(EmptyVar);
----------------
jhenderson wrote:
> Do you need to be explicit about the unsigned-ness here?
Yes to avoid a compiler warning, see earlier comment.

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:361-366
+  // Check eval fails even if we kept a pointer to the numeric expression.
+  EXPECT_FALSE(NumExpr->eval());
+  P = FileCheckPattern(Check::CheckPlain, &Cxt);
+  NumExpr =
+      P.parseNumericExpression(LocalNumVarRef, false /*IsPseudo*/, "", SM);
+  EXPECT_FALSE(NumExpr);
----------------
jhenderson wrote:
> I'm not sure I follow the purpose of this test case addition.
Numeric expressions are linked to numeric variable structure at parse time but clearing of local variable happens at runtime. It is therefore important that evaluating a cleared variable gives an error even if we have a pointer from before it was cleared. I've updated the comment to explain it.

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:386
+  EXPECT_TRUE(NumExprVal);
+  EXPECT_EQ(*NumExprVal, 36U);

----------------
jhenderson wrote:
> Is it necessary to be explicit about the unsigned-ness here?
Yes, see earlier comment.

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:398
+  EXPECT_TRUE(NumExprVal);
+  EXPECT_EQ(*NumExprVal, 36U);
 }
----------------
jhenderson wrote:
> Is it necessary to be explicit about the unsigned-ness here?
Ditto.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60385