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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth marked 4 inline comments as done.
paulkirth added a comment.

Sorry of the delay on this, I was a bit under the weather and had some other work take up more time than I planned.

I think this version makes reasonable tradeoffs between the old semantics and making use of the underlying C++ types. If there is something else that needs to be addressed, please let me know.



================
Comment at: llvm/include/llvm/Support/JSON.h:77
 
+namespace internal {
+template <typename T>
----------------
I'm not 100% on using a namespace here (or naming it `internal`), but I didn't want to introduce this into `llvm::json` by accident. 

If there is a preference, or an llvm coding preference I've missed, please let me know. https://llvm.org/docs/CodingStandards.html#anonymous-namespaces didn't seem to be relevant from my reading, but I'm happy to adopt something else here that is more in line w/ the rest of the codebase.


================
Comment at: llvm/include/llvm/Support/JSON.h:341-343
+            typename = std::enable_if_t<
+                std::is_integral_v<T> && std::is_unsigned_v<T> &&
+                !std::is_same_v<T, bool> && sizeof(T) <= sizeof(uint64_t)>>
----------------
jhenderson wrote:
> It's probably worth calling out that you've changed the behaviour somewhat. I fully expect the behaviour change to be harmless, but it's worth pointing out in the commit message.
Since I've restored the behavior, I think its best to leave them as is. 


================
Comment at: llvm/include/llvm/Support/JSON.h:426
   // Succeeds if the Value is a Number, and exactly representable as int64_t.
   std::optional<int64_t> getAsInteger() const {
     if (LLVM_LIKELY(Type == T_Integer))
----------------
jhenderson wrote:
> Are there existing unit tests for this code? I'm slightly concerned that we might accidentally break the intended behaviour without them.
Yes, these are tested in https://github.com/llvm/llvm-project/blob/0f9842c21c76a1c36866589770f92cc63619d36a/llvm/unittests/Support/JSONTest.cpp#L352. 

There are other relevant tests in the file too, for JSON Numbers, the asUINT64() API, and conversions between them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146771