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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/JSON.h:77
 
+namespace internal {
+template <typename T>
----------------
paulkirth wrote:
> 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.
I'm not sure it matters if this ends up in the `llvm::json` namespace, although perhaps it should just be in the `llvm` namespace, since it's not JSON specific.

I don't know of any specific rules here. Generally, I'd avoid anonymous namespaces (and static functions/variables) in header files, because that would result in a copy of it in every object file that uses the header, but in this particular case, I suspect it wouldn't since this is a `constexpr` value, so using an anonymous namespace (or possibly `static` - not sure if that can be used in this context), should be okay, I think?

FWIW, I'm not convinced about using an "internal" namespace.


================
Comment at: llvm/include/llvm/Support/JSON.h:339
 
   // Unsigned 64-bit long integers.
   template <typename T,
----------------
jhenderson wrote:
> You may need to update these comments.
Can we drop "long" from the comment, please?


================
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 << ": " << static_cast<int>(Value) << "\n";
----------------
I think you need a specific `char` overload too? `char` is a separate fundamental type to `signed char` and `unsigned char`.


================
Comment at: llvm/include/llvm/Support/ScopedPrinter.h:220
+  }
+  virtual void printNumber(StringRef Label, unsigned int Value) {
+    startLine() << Label << ": " << Value << "\n";
----------------
Nit: missing blank line between methods.


================
Comment at: llvm/unittests/Support/JSONTest.cpp:459
+
+// Test that underlying c++ integer types behave as expected
+TEST(JSONTest, CppIntegers) {
----------------
Nits. (I'm pretty sure "C++" is the "correct" way of writing it, but I could be wrong).


================
Comment at: llvm/unittests/Support/JSONTest.cpp:462
+  checkCppIntegers<char>();
+  checkCppIntegers<unsigned char>();
+
----------------
You need a `signed char` test case too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146771