[PATCH] D49084: FileCheck: Add support for numeric variables and expressions

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

thopre marked 5 inline comments as done.
thopre added inline comments.


================
Comment at: docs/CommandGuide/FileCheck.rst:514-518
 
-To support this case, FileCheck allows using ``[[@LINE]]``,
-``[[@LINE+<offset>]]``, ``[[@LINE-<offset>]]`` expressions in patterns. These
-expressions expand to a number of the line where a pattern is located (with an
-optional integer offset).
+To support this case, FileCheck understands the ``@LINE`` pseudo variable in
+patterns in both regular variable uses ``[[@LINE]]`` and in expressions
+``[[@LINE+<offset>]]``, ``[[@LINE-<offset>]]``. These expand to a number of
+the line where a pattern is located (with an optional integer offset).
----------------
jhenderson wrote:
> I'm wondering why this comment has changed?
It changes for 2 reasons:
- it redefines @LINE as a (pseudo) numeric variable as opposed to a pattern variable since it behaves like a numeric variable
- it removes the explanation about offset since it comes naturally from being a numeric variable and then more (eg. [[#@LINE + OFFSETVAR]]).

In short, because we now have numeric variable it is easier to say @LINE is one and say what it evaluates to and leave what can be done to it to the general description of numeric variable.


================
Comment at: include/llvm/Support/FileCheck.h:225
+  /// Pointer to AST of the numeric expression.
+  std::shared_ptr<FileCheckNumExprAST> AST;
+
----------------
arichardson wrote:
> Does this need to be a `shared_ptr`? Could it be a `unique_ptr` instead?
Several numeric expressions can refer to the same numeric variable in which case you want the reference counting so that when calling the destructor for those numeric expression delete is called only once on that variable.


================
Comment at: lib/Support/FileCheck.cpp:81
+/// representation (true if value is invalid).
+bool FileCheckNumExprVal::GetStringRepr(struct FileCheckNumExprFmtType Fmt,
+                                        std::string &StringRepr) const {
----------------
arichardson wrote:
> Could this return a `llvm::Optional<std::string>` to avoid the output-parameter?
Yes indeed and perhaps in a few other places as well. Thanks


Repository:
  rL LLVM

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

https://reviews.llvm.org/D49084