[PATCH] D146771: [support] Provide overload to PrintNumber that use C++ types

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I don't have a strong opinion on relying on conversions versus the duplicate code, so I'm happy for you to do what you prefer.



================
Comment at: llvm/include/llvm/Support/JSON.h:350
   // Integers (except boolean and uint64_t).
   // Must be non-narrowing convertible to int64_t.
+  template <
----------------
paulkirth wrote:
> I didn't love that I had to change these, but the non-narrowing issue forced me to add `unsigned long long`. I looked into just pushing all of the unsigned integers into the `uint64_t` version, but ran into some strange behavior and test failures that I couldn't explain.
> 
> For example an MCA test failed, but the JSON appeared equivalent. The difference was that despite being equivalent "numbers", the values were not integers that the `getInteger()` API for json::Objects could resolve (i.e. it relies on them being `int64_t`, and some were now `uint64_t`).
> 
> Some of that likely is tied to the JSON rules for numeric types, but someone with more knowledge here should probably weigh in. Also template metaprogramming isn't something I do often, so there may be better ways to express these constraints.
I don't think the changes you've made will work in a cross-platform manner for the same sort of reasoning as why you need the `unsigned long` overloads of printNumber: an unsigned long is a 64-bit value on some platforms, but `uint64_t` might be defined to `unsigned long long`. In such cases, unsigned long will be neither uin64_t, nor unsigned long long, and so the enable_ifs won't work.

A 64-bit integral will have the following traits: `std::is_integral`, and `sizeof(T) == sizeof(uint64_t)` (you could hard code the latter to 8). So the current behaviour of comparing the type against `uint64_t` isn't really correct. I think you want to change the unsigned case to:
```
  template <
      typename T,
      typename = std::enable_if_t<std::is_integral<T>::value &&
                                  std::is_unsigned<T>::value &&
                                  !std::is_same<T, bool>::value &&
                                  sizeof(T) == sizeof(uint64_t)>>
```
The signed one should look like the following:
```
  template <
      typename T,
      typename = std::enable_if_t<std::is_integral<T>::value &&
                                  std::is_signed<T>::value &&
                                  sizeof(T) == sizeof(int64_t)>>
```
Honestly, I'm not sure why the second one is listed as multiple `enable_if_t` args: I would have that that would be unnecessary, but if it is necessary (my own template metaprogramming sometimes needs a little bit of trial and error!), you could split the above into three separate ones.

Note that the comparison against bool is moved because `is_signed` explicitly excludes `bool`, but `is_unsigned` includes it.

You could, if you wanted, defined your own type trait that folds together the is_integral and sizeof terms, to avoid the duplication, but I'm not sure it would add enough to be worth it.

Is there any form of testing that shows that the different fundamental types create the correct underlying type?


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:201
 
-  virtual void printNumber(StringRef Label, uint64_t Value) {
-    startLine() << Label << ": " << Value << "\n";
+  virtual void printNumber(StringRef Label, signed char Value) {
+    startLine() << Label << ": " << int(Value) << "\n";
----------------
paulkirth wrote:
> Another potential approach would be to rely on conversion, and just pic a select few overloads for integer types. I usually try to avoid things like that, but 1) this is a lot of duplicated code, and 2) for many of these types the conversion is likely happening when writing to the stream.
Looking at the `Value` class above, it only has explicit overloads for `uint64_t` and `int64_t` (effectively). Other integer types happen to work because these two overloads have ended up being instantiated and so the types can convert. I might be wrong, but I suspect if you tried to use that class with e.g. an `int` value, but without any usage with an `int64_t`, you'd get a missing overload error, or something stranger (like it converting to a pointer or something)! I'm a little unsure which one `char` will use, and I suspect it's platform dependent.

In your case, I think you could get away with `long long` and `unsigned long long`, but in your testing, I'd test with every fundamental integer type to make sure it works without a problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146771