[PATCH] D66141: [FileCheck] Forbid using var defined on same line

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre marked an inline comment as done.
thopre added a comment.

In D66141#1648698 <https://reviews.llvm.org/D66141#1648698>, @jhenderson wrote:

> I just want to make sure I understand the aim here:
>
> 1. This partially reverts the behaviour introduced in rL366897 <https://reviews.llvm.org/rL366897> to prevent the issue with variables defined on the same line (but still allowing variables to be defined from an expression).
> 2. A later patch will fix and reintroduce this behaviour.


Correct.

> Is there anything in particular preventing you doing 2) straight away, making this patch redundant?

In short: having a reliable FileCheck ASAP.

I've spent some time already trying to come up with a staging mechanism to be able to set the value of a variable from an expression for the case where it's used later but revert if the line fails to match. I want a system that is both simple (less risk of bugs) and foolproof (cannot evaluate a variable without a staging area initialized and automatic rollback if not committing the result) which I didn't quite manage due to the current encapsulation. Since I also have less time to spend on FileCheck these days, that means the bug would stay there for longer that I would like. Since this bug can result in incorrect match and thus hide others, I'd really prefer to have a fix now, even if that means loosing a bit of (arguably corner case) feature.

> This is a minor point, but I wonder whether you should stop calling it "on the same line" and start saying "in the same check". If I'm not mistaken, a CHECK-SAME would allow for a usage of a variable in a different check from the one it was defined in, but matching on the same output line:
> 
>   0
>   1 2
>   CHECK:      [[#VAR1:]]
>   CHECK-NEXT: [[#VAR2:VAR1+1]]
>   CHECK-SAME: [[#VAR2+1]]

Indeed. Will fix that.



================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:86-88
-  // Variable defined by numeric expression: isValueKnownAtMatchTime
-  // returns true, getValue and eval return value of expression, setValue
-  // clears expression.
----------------
jhenderson wrote:
> I'm not sure I follow why this case has been deleted, if it's still valid to define a variable via an expression.
Indeed, it just needs adapting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66141