Re: [Lldb-commits] [PATCH] D15187: Improve the functionality of JSONNumber

[Enrico Granata via lldb-commits]

granata.enrico added a subscriber: granata.enrico.
granata.enrico added a comment.

IIU my C++ correctly, this code covers neither int64_t nor double; it covers 
the unsigned variety of int64 - as well as any other unsigned integer type.

The main advantage (other than not relying on implicit promotions) would be 
that if someone writes their new integral data type (BigNum), and mark it with 
the appropriate traits, it would Just Work

- Enrico


http://reviews.llvm.org/D15187



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D15187: Improve the functionality of JSONNumber

[Greg Clayton via lldb-commits]

clayborg added a comment.

Everything looks good, just wondering why we need the template code? See 
inlined comments.



Comment at: include/lldb/Utility/JSON.h:102-104
@@ +101,5 @@
+
+template {} &&
+  std::is_unsigned{}>::type* = 
nullptr>
+explicit JSONNumber (T u) :

What value is the template code giving us? Why can't we omit this and just have 
a constructor with int64_t and double?


http://reviews.llvm.org/D15187



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D15187: Improve the functionality of JSONNumber

[Enrico Granata via lldb-commits]

IIU my C++ correctly, this code covers neither int64_t nor double; it covers 
the unsigned variety of int64 - as well as any other unsigned integer type.

The main advantage (other than not relying on implicit promotions) would be 
that if someone writes their new integral data type (BigNum), and mark it with 
the appropriate traits, it would Just Work

- Enrico
Sent from my iPhone

> On Dec 3, 2015, at 9:34 AM, Greg Clayton  wrote:
> 
> clayborg added a comment.
> 
> Everything looks good, just wondering why we need the template code? See 
> inlined comments.
> 
> 
> 
> Comment at: include/lldb/Utility/JSON.h:102-104
> @@ +101,5 @@
> +
> +template  +  typename std::enable_if +  std::is_unsigned{}>::type* = 
> nullptr>
> +explicit JSONNumber (T u) :
> 
> What value is the template code giving us? Why can't we omit this and just 
> have a constructor with int64_t and double?
> 
> 
> http://reviews.llvm.org/D15187
> 
> 
> 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D15187: Improve the functionality of JSONNumber

[Tamas Berghammer via lldb-commits]

tberghammer added inline comments.


Comment at: include/lldb/Utility/JSON.h:102-104
@@ +101,5 @@
+
+template {} &&
+  std::is_unsigned{}>::type* = 
nullptr>
+explicit JSONNumber (T u) :

clayborg wrote:
> What value is the template code giving us? Why can't we omit this and just 
> have a constructor with int64_t and double?
If we have a constructor with int64_t and one with double then we will get a 
compilation error when we try to call it with an int32_t because the 2 overload 
will be ambiguous. We can work it around at each call site with casting the 
value to the type what the constructor takes but I would prefer to handle the 
issue it in 1 place instead of all call site.


http://reviews.llvm.org/D15187



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D15187: Improve the functionality of JSONNumber

[Greg Clayton via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

Ok, thanks for the explanation. You might add a comment saying something like 
you just said above the template stuff so people can see why it is needed.


http://reviews.llvm.org/D15187



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits