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

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

thopre added inline comments.


================
Comment at: include/llvm/Support/FileCheck.h:81
+
+namespace Check {
+/// Phases of FileCheck.  Used to determine at what phase is a numeric
----------------
jhenderson wrote:
> Don't namespace this. Just make the enum inside a scoped enum (i.e. `enum class`).
I was following what is done for the Check enum. Should that enum be changed (in a separate patch)?


================
Comment at: include/llvm/Support/FileCheck.h:82-84
+/// Phases of FileCheck.  Used to determine at what phase is a numeric
+/// expression (and thus value of any potential variable defined by it)
+/// available.
----------------
jhenderson wrote:
> I think the general style in LLVM is single space after full stop (applies here and elsewhere).
> 
> "is a numeric expression (and thus value ...) available" -> "a numeric expression is available (and thus the value ...)"
I saw many occurrences of 2 spaces after dot in FileCheck and thought it had a different code style. Upon inspection I suppose it must be mistakes as single space is more common:

$ git grep -E -c "\. [^ ]" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/lib/Support/FileCheck.cpp 
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/lib/Support/FileCheck.cpp:20
$ git grep -E -c "\.  [^ ]" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/lib/Support/FileCheck.cpp 
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/lib/Support/FileCheck.cpp:12

$ git grep -E -c "\. [^ ]" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/include/llvm/Support/FileCheck.h
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/include/llvm/Support/FileCheck.h:4
$ git grep -E -c "\.  [^ ]" fcefe03bf94005e045499ed6650212cd1e9bed01 llvm/include/llvm/Support/FileCheck.h
fcefe03bf94005e045499ed6650212cd1e9bed01:llvm/include/llvm/Support/FileCheck.h:3


================
Comment at: include/llvm/Support/FileCheck.h:97
+/// and return both signed and unsigned value.
+class FileCheckNumExprVal {
+private:
----------------
jhenderson wrote:
> I'm not sure you need to name all your classes "FileCheckXXX". You can shorten their names by dropping the FileCheck part, I reckon.
The FileCheck prefix was added when FileCheck logic was moved into a library. See comment from Bogner in https://reviews.llvm.org/D50283


================
Comment at: include/llvm/Support/FileCheck.h:99-103
+  /// Signed value.
+  int64_t SVal;
+
+  /// Unsigned value.
+  uint64_t UVal;
----------------
jhenderson wrote:
> Why not just always store in a single uint64_t field called `Value` and convert to signed when requested?
> 
> Also, don't unnecessarily abbreviate variables like this.
Since unsigned to signed conversion is implementation defined and reinterpret_cast does not work between integer types, I would have to do the two-complement manually. I've used a union instead, hope you like it better.


================
Comment at: include/llvm/Support/FileCheck.h:139
+  /// Whether value is signed.
+  bool IsSigned() const { return Signed; }
+
----------------
jhenderson wrote:
> LLVM style is for camel-case, starting with lower-case letters for method and function names. Also, don't bother commenting if the function purpose is obvious from its name.
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?



================
Comment at: include/llvm/Support/FileCheck.h:209-213
+  /// Constructor for a signed literal.
+  FileCheckNumExprLiteral(int64_t Val) : Value(Val) {}
+
+  /// Constructor for an unsigned literal.
+  FileCheckNumExprLiteral(uint64_t Val) : Value(Val) {}
----------------
jhenderson wrote:
> Similar to the FileCheckNumExprVal class, I don't think you need both constructors. One should suffice.
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.


================
Comment at: include/llvm/Support/FileCheck.h:294-299
+  /// Store in \p EvaluatedValue the value evaluated from the constraint of
+  /// this numeric expression.  Return whether evaluation failed.
+  bool Eval(FileCheckNumExprVal &EvaluatedValue) const;
+
+  /// Set Value from the result of calling Eval().  Return whether it failed.
+  bool SetValueFromEval();
----------------
jhenderson wrote:
> Why are these two different functions?
I can't remember now except that I needed it at some point. It is clearly not needed now, maybe as a result of having the known phase info. Anyway, merged into one.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D49084