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

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

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


================
Comment at: llvm/lib/Support/FileCheck.cpp:110
+  if (Signed)
+    return APInt(sizeof(Value) * CHAR_BIT, Value, Signed).getSExtValue();
+
----------------
arichardson wrote:
> 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.
Value is uint64_t and C++14 says:


```
If the destination type is signed, the value is unchanged if it can be represented in the destination type;otherwise, the value is implementation-defined
```

So if Value corresponds to a negative signed integer the conversion is implementation-defined. I don't know how APInt deal with this but I hope it does something C++14 compliant.


================
Comment at: llvm/lib/Support/FileCheck.cpp:133
+
+  return ExpressionValue(~Value + 1, /*Signed=*/false);
+}
----------------
arichardson wrote:
> 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)`?
As per above, the conversion to (int64_t) is implementation-defined in case where Value has its msb set (ie. corresponds to a negative signed value).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60390