You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2015/09/10 00:20:33 UTC
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/
-----------------------------------------------------------
(Updated Sept. 9, 2015, 3:20 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
Changes
-------
Deal with `uint64_t` gracefully (i.e. without completely losing the value during conversion).
Summary (updated)
-----------------
[3/5] Integer Precision for JSON <-> Protobuf conversions.
Bugs: MESOS-3345
https://issues.apache.org/jira/browse/MESOS-3345
Repository: mesos
Description (updated)
-------
* Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
* Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
* Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
* Added one test for JSON::Number comparisons.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
Diff: https://reviews.apache.org/r/38031/diff/
Testing (updated)
-------
No testing done until the last patch in the chain.
However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
Thanks,
Joseph Wu
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Sept. 9, 2015, 3:24 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp, lines 103-110
> > <https://reviews.apache.org/r/38031/diff/3/?file=1066423#file1066423line103>
> >
> > Let's make this overlapped storage private and add accessors. We can postfix `_` to distinguish the variable names of the storage so that we can write the functions like this:
> >
> > ```
> > double value() const
> > {
> > CHECK(type == FLOATING) << error_msg;
> > return value_;
> > }
> > ```
> >
> > This has the advantage of forcing us to change all the usage sites and make sure we make the right decision at each point.
I thought we didn't want to change all the tests (which is where we find usages of `.value`). That's why the double printing was changed instead.
Also, can you take another look at the accessor?
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review98277
-----------------------------------------------------------
On Sept. 9, 2015, 3:20 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2015, 3:20 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review98277
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 95 - 101)
<https://reviews.apache.org/r/38031/#comment154616>
You can do:
```
enum Type
{
FLOATING,
INTEGER,
} type;
```
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 103 - 110)
<https://reviews.apache.org/r/38031/#comment154630>
Let's make this overlapped storage private and add accessors. We can postfix `_` to distinguish the variable names of the storage so that we can write the functions like this:
```
double value() const
{
CHECK(type == FLOATING) << error_msg;
return value_;
}
```
This has the advantage of forcing us to change all the usage sites and make sure we make the right decision at each point.
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 418)
<https://reviews.apache.org/r/38031/#comment154619>
an we call this `number` instead of `myself`?
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 485)
<https://reviews.apache.org/r/38031/#comment154620>
can we call this `number` and maybe call the incoming parameter `other` to be consistent in the file?
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 547)
<https://reviews.apache.org/r/38031/#comment154621>
Should we initialize this like this?
```
char buffer[50] {};
```
- Joris Van Remoortere
On Sept. 9, 2015, 10:20 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2015, 10:20 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review98901
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 110 - 111)
<https://reviews.apache.org/r/38031/#comment155598>
Let's replace the default cases with a comment that explains we prefer the compiler generated warning for a missing case. Here and elsewhere.
(As per discussion offline)
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 441 - 482)
<https://reviews.apache.org/r/38031/#comment155596>
We can delegate this to the Comparator since they perform the same logic.
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 461 - 463)
<https://reviews.apache.org/r/38031/#comment155600>
Can you add a comment here and below where the behavior isn't quite a "dummy" equality check?
It is easy to glance over what you are doing here.
- Joris Van Remoortere
On Sept. 11, 2015, 5:42 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2015, 5:42 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review99058
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Sept. 15, 2015, 5:24 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2015, 5:24 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Sept. 16, 2015, 11:01 a.m., Ben Mahler wrote:
> > Have you surveyed how other libraires handle this? For example, the scala play framework seems to use BigDecimal (128 bit floating point) for all numbers, which can represent all 64 bit signed / unsigned integral values without loss. Have you considered this as well?
> >
> > It would be great to outline the options we have here in the ticket so we can decide on the tradeoffs. If you've already done this, can you point me to it? :)
>
> Joris Van Remoortere wrote:
> Hi Ben! THanks for bringing this up.
>
> When you say "handle this", what do you mean exactly? The issue that Joseph is dealing with is the round-trip loss when we transit through pico-json. The modification to Number are just to maintain type information when possible.
>
> I believe we would have to choose another parsing library if we wanted to use 128 bit floats, or am I missing something? I'm definitely open to discussing alternative libraries; but I would like to patch this up in the mean time so that we don't need to rush the larger discussion. What are your thoughts?
>
> Ben Mahler wrote:
> Improving over the current state sounds good, just trying to gauge if this is how we want this to look long term: Looking at the diff, it seems like we still are not able to represent every unsigned 64 bit integer? I'm a bit worried about that, we can get away with it for now, but given we may use uint64s for representing identifiers, this needs to be non-lossy longer term, no?
Ben, are you seeing a problem with large uint64's in this diff? If so, I'd like to fix it.
As Joris (and I, in the code) mentioned, the loss that occurs between INT64_MAX and UINT64_MAX is due to a limitation within PicoJson. We would need to change/patch the json library to fix this. (If this is the case, I'd recommend moving this dicussion to the JIRA.)
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review99254
-----------------------------------------------------------
On Sept. 15, 2015, 10:24 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2015, 10:24 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 16, 2015, 6:01 p.m., Ben Mahler wrote:
> > Have you surveyed how other libraires handle this? For example, the scala play framework seems to use BigDecimal (128 bit floating point) for all numbers, which can represent all 64 bit signed / unsigned integral values without loss. Have you considered this as well?
> >
> > It would be great to outline the options we have here in the ticket so we can decide on the tradeoffs. If you've already done this, can you point me to it? :)
>
> Joris Van Remoortere wrote:
> Hi Ben! THanks for bringing this up.
>
> When you say "handle this", what do you mean exactly? The issue that Joseph is dealing with is the round-trip loss when we transit through pico-json. The modification to Number are just to maintain type information when possible.
>
> I believe we would have to choose another parsing library if we wanted to use 128 bit floats, or am I missing something? I'm definitely open to discussing alternative libraries; but I would like to patch this up in the mean time so that we don't need to rush the larger discussion. What are your thoughts?
>
> Ben Mahler wrote:
> Improving over the current state sounds good, just trying to gauge if this is how we want this to look long term: Looking at the diff, it seems like we still are not able to represent every unsigned 64 bit integer? I'm a bit worried about that, we can get away with it for now, but given we may use uint64s for representing identifiers, this needs to be non-lossy longer term, no?
>
> Joseph Wu wrote:
> Ben, are you seeing a problem with large uint64's in this diff? If so, I'd like to fix it.
>
> As Joris (and I, in the code) mentioned, the loss that occurs between INT64_MAX and UINT64_MAX is due to a limitation within PicoJson. We would need to change/patch the json library to fix this. (If this is the case, I'd recommend moving this dicussion to the JIRA.)
> the loss that occurs between INT64_MAX and UINT64_MAX is due to a limitation within PicoJson.
This is what I was referring to when I mentioned not being able to represent every uint64.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review99254
-----------------------------------------------------------
On Sept. 15, 2015, 5:24 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2015, 5:24 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joris Van Remoortere <jo...@gmail.com>.
> On Sept. 16, 2015, 6:01 p.m., Ben Mahler wrote:
> > Have you surveyed how other libraires handle this? For example, the scala play framework seems to use BigDecimal (128 bit floating point) for all numbers, which can represent all 64 bit signed / unsigned integral values without loss. Have you considered this as well?
> >
> > It would be great to outline the options we have here in the ticket so we can decide on the tradeoffs. If you've already done this, can you point me to it? :)
Hi Ben! THanks for bringing this up.
When you say "handle this", what do you mean exactly? The issue that Joseph is dealing with is the round-trip loss when we transit through pico-json. The modification to Number are just to maintain type information when possible.
I believe we would have to choose another parsing library if we wanted to use 128 bit floats, or am I missing something? I'm definitely open to discussing alternative libraries; but I would like to patch this up in the mean time so that we don't need to rush the larger discussion. What are your thoughts?
- Joris
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review99254
-----------------------------------------------------------
On Sept. 15, 2015, 5:24 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2015, 5:24 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Ben Mahler <be...@gmail.com>.
> On Sept. 16, 2015, 6:01 p.m., Ben Mahler wrote:
> > Have you surveyed how other libraires handle this? For example, the scala play framework seems to use BigDecimal (128 bit floating point) for all numbers, which can represent all 64 bit signed / unsigned integral values without loss. Have you considered this as well?
> >
> > It would be great to outline the options we have here in the ticket so we can decide on the tradeoffs. If you've already done this, can you point me to it? :)
>
> Joris Van Remoortere wrote:
> Hi Ben! THanks for bringing this up.
>
> When you say "handle this", what do you mean exactly? The issue that Joseph is dealing with is the round-trip loss when we transit through pico-json. The modification to Number are just to maintain type information when possible.
>
> I believe we would have to choose another parsing library if we wanted to use 128 bit floats, or am I missing something? I'm definitely open to discussing alternative libraries; but I would like to patch this up in the mean time so that we don't need to rush the larger discussion. What are your thoughts?
Improving over the current state sounds good, just trying to gauge if this is how we want this to look long term: Looking at the diff, it seems like we still are not able to represent every unsigned 64 bit integer? I'm a bit worried about that, we can get away with it for now, but given we may use uint64s for representing identifiers, this needs to be non-lossy longer term, no?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review99254
-----------------------------------------------------------
On Sept. 15, 2015, 5:24 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2015, 5:24 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review99254
-----------------------------------------------------------
Have you surveyed how other libraires handle this? For example, the scala play framework seems to use BigDecimal (128 bit floating point) for all numbers, which can represent all 64 bit signed / unsigned integral values without loss. Have you considered this as well?
It would be great to outline the options we have here in the ticket so we can decide on the tradeoffs. If you've already done this, can you point me to it? :)
- Ben Mahler
On Sept. 15, 2015, 5:24 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2015, 5:24 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/
-----------------------------------------------------------
(Updated Sept. 17, 2015, 12:14 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
Changes
-------
Re-opening with fixes for g++ 4.8/4.9 on CentOS 7.
Bugs: MESOS-3345
https://issues.apache.org/jira/browse/MESOS-3345
Repository: mesos
Description
-------
* Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
* Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
* Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
* Added one test for JSON::Number comparisons.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 115cc0cbec04db1bd9e068263f3931f33c266e86
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 87737dd1669ee77db2f29dacd8b5cebb0cabc0e6
Diff: https://reviews.apache.org/r/38031/diff/
Testing
-------
No testing done until the last patch in the chain.
However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
Thanks,
Joseph Wu
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/
-----------------------------------------------------------
(Updated Sept. 15, 2015, 10:24 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
Changes
-------
Changed spacing on one line. Renamed one test.
Bugs: MESOS-3345
https://issues.apache.org/jira/browse/MESOS-3345
Repository: mesos
Description
-------
* Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
* Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
* Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
* Added one test for JSON::Number comparisons.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
Diff: https://reviews.apache.org/r/38031/diff/
Testing
-------
No testing done until the last patch in the chain.
However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
Thanks,
Joseph Wu
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Michael Park <mc...@gmail.com>.
> On Sept. 15, 2015, 8:41 a.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp, line 128
> > <https://reviews.apache.org/r/38031/diff/7/?file=1072999#file1072999line128>
> >
> > Haha! Perhaps the most subtle C++11 feature.
>
> Joseph Wu wrote:
> You mean the comma at the end? :)
Yep :P
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review98998
-----------------------------------------------------------
On Sept. 15, 2015, 5:24 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2015, 5:24 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Sept. 15, 2015, 1:41 a.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp, line 128
> > <https://reviews.apache.org/r/38031/diff/7/?file=1072999#file1072999line128>
> >
> > Haha! Perhaps the most subtle C++11 feature.
You mean the comma at the end? :)
> On Sept. 15, 2015, 1:41 a.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp, line 141
> > <https://reviews.apache.org/r/38031/diff/7/?file=1073002#file1073002line141>
> >
> > We typically don't put underscores in our test names. They're usually reserved for things like `DISABLED_*` and `ROOT_*`.
I'll rename it to `JsonLargeIntegers`. (I put the underscore there because of the capital `JSON`.)
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review98998
-----------------------------------------------------------
On Sept. 14, 2015, 1:37 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 1:37 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review98998
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 128)
<https://reviews.apache.org/r/38031/#comment155801>
Haha! Perhaps the most subtle C++11 feature.
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 134 - 136)
<https://reviews.apache.org/r/38031/#comment155799>
This fits in one line.
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (line 141)
<https://reviews.apache.org/r/38031/#comment155803>
We typically don't put underscores in our test names. They're usually reserved for things like `DISABLED_*` and `ROOT_*`.
- Michael Park
On Sept. 14, 2015, 8:37 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 8:37 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/
-----------------------------------------------------------
(Updated Sept. 14, 2015, 1:37 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
Changes
-------
Slight refactoring of switch blobs. Removed default switch cases. Added some notes/comments.
Bugs: MESOS-3345
https://issues.apache.org/jira/browse/MESOS-3345
Repository: mesos
Description
-------
* Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
* Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
* Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
* Added one test for JSON::Number comparisons.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
Diff: https://reviews.apache.org/r/38031/diff/
Testing
-------
No testing done until the last patch in the chain.
However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
Thanks,
Joseph Wu
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review98910
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 571 - 572)
<https://reviews.apache.org/r/38031/#comment155623>
Let's add comments as to why we need this here but not for doubles.
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 650)
<https://reviews.apache.org/r/38031/#comment155620>
`s/50/sizeof(buffer)/`
- Michael Park
On Sept. 11, 2015, 5:42 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2015, 5:42 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review97421
-----------------------------------------------------------
For some reason this did not get posted when it should have.
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 85 - 87)
<https://reviews.apache.org/r/38031/#comment153303>
Could you please provide more details here? Or maybe just post a like to PicoJSON docs.
- Artem Harutyunyan
On Sept. 11, 2015, 10:42 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2015, 10:42 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/#review98906
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 89 - 90)
<https://reviews.apache.org/r/38031/#comment155613>
Typically, we would do something like:
```cpp
typename std::enable_if<std::is_integral<T>::value && std::is_signed<T>::value, int>::type = 0
```
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 96 - 97)
<https://reviews.apache.org/r/38031/#comment155614>
Same here.
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 117 - 119)
<https://reviews.apache.org/r/38031/#comment155616>
Can we mention the `SIGNED` part as well?
(`SIGNED_INTEGER` and `UNSIGNED_INTEGER`) or (`SIGNED` and `UNSIGNED`)?
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 125)
<https://reviews.apache.org/r/38031/#comment155617>
Is the `inline` necessary here?
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (line 126)
<https://reviews.apache.org/r/38031/#comment155618>
We typically call this `stream`.
- Michael Park
On Sept. 11, 2015, 5:42 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> -----------------------------------------------------------
>
> (Updated Sept. 11, 2015, 5:42 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> * Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
> * Added one test for JSON::Number comparisons.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
> 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
>
> Diff: https://reviews.apache.org/r/38031/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
> However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/
-----------------------------------------------------------
(Updated Sept. 11, 2015, 10:42 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
Changes
-------
Rebase due to test fix and merge conflict.
Bugs: MESOS-3345
https://issues.apache.org/jira/browse/MESOS-3345
Repository: mesos
Description
-------
* Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
* Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
* Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
* Added one test for JSON::Number comparisons.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
Diff: https://reviews.apache.org/r/38031/diff/
Testing
-------
No testing done until the last patch in the chain.
However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
Thanks,
Joseph Wu
Re: Review Request 38031: [3/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38031/
-----------------------------------------------------------
(Updated Sept. 10, 2015, 11:25 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Michael Park, and Vinod Kone.
Changes
-------
Address Joris's comments. Made the JSON::Number's storage fields private.
Bugs: MESOS-3345
https://issues.apache.org/jira/browse/MESOS-3345
Repository: mesos
Description
-------
* Changes JSON::Number to keep track of whether it is floating, signed integral, or unsigned integral.
* Changes how JSON::Number is stringified, to ensure that stringified doubles are parsed as JSON doubles.
* Added one test for integer precision between String <-> JSON <-> Protobuf conversions.
* Added one test for JSON::Number comparisons.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf45273c620655b44b5f5572290cffa4bf6
3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c269e9be24c0f1ae81b8aa8725f8a0c151
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3098293eb3659b3066f10e875927ec3ac3
Diff: https://reviews.apache.org/r/38031/diff/
Testing
-------
No testing done until the last patch in the chain.
However, this patch does breaks some libprocess and mesos tests (because JSON::Number is stored differently), which are fixed in the following two reviews.
Thanks,
Joseph Wu