[PATCH] D60390: FileCheck [10/12]: Add support for signed numeric values

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:110
+  if (Signed)
+    return APInt(sizeof(Value) * CHAR_BIT, Value, Signed).getSExtValue();
+
----------------
jhenderson wrote:
> I'm a little confused as to what is going on here, as I'm not up-to-speed on APInt. Could you give a brief explanation, please?
Using APInt here seems unnecessary. How about `return (int64_t)Value;`?

Or if you would like to avoid the casts you could use a union.


================
Comment at: llvm/lib/Support/FileCheck.cpp:133
+
+  return ExpressionValue(~Value + 1, /*Signed=*/false);
+}
----------------
jhenderson wrote:
> Doesn't this assume 2s complement, which isn't standard-guaranteed as far as I'm aware? I'm okay with that, if there is precendent for it in LLVM already, since it seems like C++ is moving in that direction (see http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r1.html), but I'd be nervous otherwise.
Since we know the value is signed after `if (!Signed)`, can't we change these three lines to
`return ExpressionValue(std::abs((int64_t)Value), /*Signed=*/false)`?


================
Comment at: llvm/lib/Support/FileCheckImpl.h:127
+  bool isNegative() const {
+    return Signed && (Value & (1LLU << (sizeof(Value) * CHAR_BIT - 1)));
+  }
----------------
`Signed && (int64_t)Value < 0`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60390