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

[Junhee Yoo via Phabricator via llvm-commits]

junhee-yoo added inline comments.


================
Comment at: llvm/include/llvm/Support/JSON.h:353
+      typename = std::enable_if_t<std::is_integral_v<T> && std::is_signed_v<T>>,
+      typename = std::enable_if_t<sizeof(T) <= sizeof(int64_t)>>
   Value(T I) : Type(T_Integer) {
----------------
I guess this `sizeof(T) <= sizeof(int64_t)` and `sizeof(T) <= sizeof(uint64_t)` are not affect because all types are equal or smaller than 64bits types.

In my humble opinion, how about keep the intention of original code:

- for unsigned 64bits types - uint64_t:
```
template <typename T,
          typename = std::enable_if_t<!std::is_same_v<T, bool>&&
                                      std::is_integral_v<T> &&
                                      std::is_unsigned_v<T> &&
                                      sizeof(T) == sizeof(uint64_t)>>
```

- for rests - int64_t:
```
template <typename T,
          typename = std::enable_if_t<std::is_integral_v<T>>,
          typename = std::enable_if_t<!std::is_same_v<T, bool>>,
          typename = std::enable_if_t<std::is_integral_v<T> &&
                                      !(std::is_unsigned_v<T> &&
                                        sizeof(T) == sizeof(uint64_t))>>
```

If it looks too verbose, I recommend to use:
```
template<typenameT>
constexpr bool is_uint_64_bits_v = 
  std::is_integral_v<T> && std::is_unsigned_v<T> && sizeof(T) == sizeof(uint64_t);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146771