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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

For some reason the new diff has lost all of the inline comments...

> I was following what is done for the Check enum. Should that enum be changed (in a separate patch)?

Yes, probably.

> The FileCheck prefix was added when FileCheck logic was moved into a library. See comment from Bogner in https://reviews.llvm.org/D50283

Ah, I hadn't noticed that it had moved to Support. Okay, this is fine as is.

> There are actually more examples of camel-case starting with capital letter: all the CheckFoo (eg. CheckSame), the PrintFoo (eg. PrintMatch).
> 
> $ git grep -E -c "[a-zA-Z]+ [a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/include/llvm/Support/FileCheck.h
>  fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/include/llvm/Support/FileCheck.h:7
>  $ git grep -E -c "[a-zA-Z]+ [a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/lib/Support/FileCheck.cpp
>  <no result>
> 
> $ git grep -E -c "[a-zA-Z]+ [A-Z][a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/include/llvm/Support/FileCheck.h
>  fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/include/llvm/Support/FileCheck.h:17
>  $ git grep -E -c "[a-zA-Z]+ [A-Z][a-z]+[A-Z][a-zA-Z]+\(" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/lib/Support/FileCheck.cpp
>  fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/lib/Support/FileCheck.cpp:13
> 
> Do you still want me to fix that?

As there are already a mixture, we should follow the LLVM style for the new code, especially given the amount of new code being added. If it was all a different style there might be a different case to be made.

---

I definitely think that you need to fragment this up into small incremental patches, as I've found I'm not getting enough time to digest the whole thing at once (and I'm guessing the same goes for other reviewers). If you could try to find incremental ways of doing things that would be great. One example would be to add the command-line switches as non-functional switches first (make them hidden until you're ready for them to be published), then incrementally add features (and expand testing) to them. Similarly accepting some forms of the pattern etc could be done in one change (but do nothing with them) and then add more functionality over time.



================
Comment at: llvm/include/llvm/Support/FileCheck.h:109
+
+  /// Whether this is a tentative value, ie other expressions on the same CHECK
+  /// line that might be using that value have not yet been verified to match.
----------------
Nit: ie -> i.e.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:129
+  /// the same signedness and corresponding value.
+  bool operator==(const FileCheckNumExprVal &other);
+  bool operator!=(const FileCheckNumExprVal &other) {
----------------
Nit: other -> Other


================
Comment at: llvm/include/llvm/Support/FileCheck.h:174-177
+  friend FileCheckNumExprVal operator+(const FileCheckNumExprVal &lhs,
+                                       const FileCheckNumExprVal &rhs);
+  friend FileCheckNumExprVal operator-(const FileCheckNumExprVal &lhs,
+                                       const FileCheckNumExprVal &rhs);
----------------
Why not make these member functions? I.e:
```
FileCheckNumExprVal operator+(const FileChcekNumExprVal &Other);
FileCheckNumExprVal operator-(const FileChcekNumExprVal &Other);
```
Also Nit: lhs/rhs should be LHS/RHS (but this is moot if switching to member operators).


================
Comment at: llvm/include/llvm/Support/FileCheck.h:191
+  /// Append names of undefined variables used in the expression represented by
+  /// this AST. Must be overriden in any subclass representing an expression
+  /// that can contain a variable.
----------------
Nit: overriden -> overridden.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:205-208
+  FileCheckNumExprLiteral(int64_t Val) : Value(Val) {}
+
+  /// Constructor for an unsigned literal.
+  FileCheckNumExprLiteral(uint64_t Val) : Value(Val) {}
----------------
> My same concern applies here: I can easily "reinterpret_cast" to a uint64_t parameter when calling the constructor and pass an extra Signed parameter but then to do signed arithmetic I cannot think of an *easy* way to reinterpret it back into an int64_t without tripping on undefined or implementation defined behaviour.
Okay, fair enough. I wonder if they could be templated then instead? I.e:
```
template <typename T> FileCheckNumExprLiteral(T Val) : Value(Val) {}
```

Also, should these be `explicit`, or are you deliberately allowing implicit conversions?


================
Comment at: llvm/include/llvm/Support/FileCheck.h:221-223
+  /// Value of the numeric expression. Set either by evaluating the AST if any
+  /// and using only variable defined on previous CHECK line, or by the matched
+  /// value if constraint is satisfied.
----------------
The second sentence is hard to parse. Could you maybe break it up into the different bits, as separate sentences?


================
Comment at: llvm/include/llvm/Support/FileCheck.h:226
+
+  /// Matching format, i.e format (eg. hex upper case letters) to use for the
+  /// value when matching it.
----------------
Nit: eg. -> e.g.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:245
+  /// Return pointer to AST of the numeric expression. Pointer is guaranteed to
+  /// be valid as long as this object is.
+  FileCheckNumExprAST *GetAST() const { return AST.get(); }
----------------
is -> is alive.


================
Comment at: llvm/include/llvm/Support/FileCheck.h:264
+  /// according to matching format of this numeric expression or nothing if
+  /// \p StrVal correspond to a valid and representable value.
+  llvm::Optional<FileCheckNumExprVal>
----------------
correspond -> corresponds


Repository:
  rL LLVM

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

https://reviews.llvm.org/D49084