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:20 UTC
Re: Review Request 38030: [2/5] Integer Precision for JSON <->
Protobuf conversions.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38030/
-----------------------------------------------------------
(Updated Sept. 9, 2015, 3:20 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
Changes
-------
Ready for wider review.
Summary (updated)
-----------------
[2/5] Integer Precision for JSON <-> Protobuf conversions.
Bugs: MESOS-3345
https://issues.apache.org/jira/browse/MESOS-3345
Repository: mesos
Description
-------
Add PICOJSON_USE_INT64 flag to mesos compilation flags.
Diffs
-----
src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5
Diff: https://reviews.apache.org/r/38030/diff/
Testing (updated)
-------
No testing done until the last patch in the chain.
Thanks,
Joseph Wu
Re: Review Request 38030: [2/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/38030/
-----------------------------------------------------------
(Updated Sept. 17, 2015, 2:01 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
Changes
-------
Add note above extra flags.
Bugs: MESOS-3345
https://issues.apache.org/jira/browse/MESOS-3345
Repository: mesos
Description
-------
Add `PICOJSON_USE_INT64` and `__STDC_FORMAT_MACROS` flag to mesos compilation flags.
Diffs (updated)
-----
src/Makefile.am 2286366852b272c808490b6bd87ac9e894de57ac
Diff: https://reviews.apache.org/r/38030/diff/
Testing
-------
No testing done until the last patch in the chain.
Thanks,
Joseph Wu
Re: Review Request 38030: [2/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 17, 2015, 7:17 p.m., Jie Yu wrote:
> > src/Makefile.am, line 111
> > <https://reviews.apache.org/r/38030/diff/4/?file=1076134#file1076134line111>
> >
> > Have you guys looked at the code in the picojson?
> > https://github.com/kazuho/picojson/blob/master/picojson.h#L61
> >
> > Looks like that macro should be defined. I don't understand why this fixed the issue.
>
> Joseph Wu wrote:
> So the issue is that other libraries like glog and protobuf have an `#include <inttypes.h>` too. But the flag isn't defined when those libraries call it.
> Later on, when we include `picojson.h`, the fact that it defines the flag doesn't do anything, since `inttypes.h` has already been included.
>
> I added a note in the next review to explain why we have an #undef (top of the diff). Would a similar note be appropriate here?
Aha, got it. Wondering if that'll break glog/protobuf because they are not expecting this Macro to be defined?
Definitely worth adding what you've just said to the code as a comment/note.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38030/#review99423
-----------------------------------------------------------
On Sept. 17, 2015, 7:13 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38030/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2015, 7:13 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add `PICOJSON_USE_INT64` and `__STDC_FORMAT_MACROS` flag to mesos compilation flags.
>
>
> Diffs
> -----
>
> src/Makefile.am 2286366852b272c808490b6bd87ac9e894de57ac
>
> Diff: https://reviews.apache.org/r/38030/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38030: [2/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Sept. 17, 2015, 12:17 p.m., Jie Yu wrote:
> > src/Makefile.am, line 111
> > <https://reviews.apache.org/r/38030/diff/4/?file=1076134#file1076134line111>
> >
> > Have you guys looked at the code in the picojson?
> > https://github.com/kazuho/picojson/blob/master/picojson.h#L61
> >
> > Looks like that macro should be defined. I don't understand why this fixed the issue.
So the issue is that other libraries like glog and protobuf have an `#include <inttypes.h>` too. But the flag isn't defined when those libraries call it.
Later on, when we include `picojson.h`, the fact that it defines the flag doesn't do anything, since `inttypes.h` has already been included.
I added a note in the next review to explain why we have an #undef (top of the diff). Would a similar note be appropriate here?
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38030/#review99423
-----------------------------------------------------------
On Sept. 17, 2015, 12:13 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38030/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2015, 12:13 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add `PICOJSON_USE_INT64` and `__STDC_FORMAT_MACROS` flag to mesos compilation flags.
>
>
> Diffs
> -----
>
> src/Makefile.am 2286366852b272c808490b6bd87ac9e894de57ac
>
> Diff: https://reviews.apache.org/r/38030/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38030: [2/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Sept. 17, 2015, 12:17 p.m., Jie Yu wrote:
> > src/Makefile.am, line 111
> > <https://reviews.apache.org/r/38030/diff/4/?file=1076134#file1076134line111>
> >
> > Have you guys looked at the code in the picojson?
> > https://github.com/kazuho/picojson/blob/master/picojson.h#L61
> >
> > Looks like that macro should be defined. I don't understand why this fixed the issue.
>
> Joseph Wu wrote:
> So the issue is that other libraries like glog and protobuf have an `#include <inttypes.h>` too. But the flag isn't defined when those libraries call it.
> Later on, when we include `picojson.h`, the fact that it defines the flag doesn't do anything, since `inttypes.h` has already been included.
>
> I added a note in the next review to explain why we have an #undef (top of the diff). Would a similar note be appropriate here?
>
> Jie Yu wrote:
> Aha, got it. Wondering if that'll break glog/protobuf because they are not expecting this Macro to be defined?
>
> Definitely worth adding what you've just said to the code as a comment/note.
I think it's unlikely that these extra macros will break anything. It does add a bunch of items to the global namespace (See: http://pubs.opengroup.org/onlinepubs/009695399/basedefs/inttypes.h.html). But the naming is odd/idiosyncratic enough that it's probably not going to conflict with anything that follows the google style guide.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38030/#review99423
-----------------------------------------------------------
On Sept. 17, 2015, 2:01 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38030/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2015, 2:01 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add `PICOJSON_USE_INT64` and `__STDC_FORMAT_MACROS` flag to mesos compilation flags.
>
>
> Diffs
> -----
>
> src/Makefile.am 2286366852b272c808490b6bd87ac9e894de57ac
>
> Diff: https://reviews.apache.org/r/38030/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38030: [2/5] Integer Precision for JSON <->
Protobuf conversions.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38030/#review99423
-----------------------------------------------------------
src/Makefile.am (line 111)
<https://reviews.apache.org/r/38030/#comment156314>
Have you guys looked at the code in the picojson?
https://github.com/kazuho/picojson/blob/master/picojson.h#L61
Looks like that macro should be defined. I don't understand why this fixed the issue.
- Jie Yu
On Sept. 17, 2015, 7:13 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38030/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2015, 7:13 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add `PICOJSON_USE_INT64` and `__STDC_FORMAT_MACROS` flag to mesos compilation flags.
>
>
> Diffs
> -----
>
> src/Makefile.am 2286366852b272c808490b6bd87ac9e894de57ac
>
> Diff: https://reviews.apache.org/r/38030/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 38030: [2/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/38030/
-----------------------------------------------------------
(Updated Sept. 17, 2015, 12:13 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 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 (updated)
-------
Add `PICOJSON_USE_INT64` and `__STDC_FORMAT_MACROS` flag to mesos compilation flags.
Diffs (updated)
-----
src/Makefile.am 2286366852b272c808490b6bd87ac9e894de57ac
Diff: https://reviews.apache.org/r/38030/diff/
Testing
-------
No testing done until the last patch in the chain.
Thanks,
Joseph Wu
Re: Review Request 38030: [2/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/38030/
-----------------------------------------------------------
(Updated Sept. 11, 2015, 10:42 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 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
-------
Add PICOJSON_USE_INT64 flag to mesos compilation flags.
Diffs (updated)
-----
src/Makefile.am 8963cea9fd7e3bee450a1059e6383e5ab868a17b
Diff: https://reviews.apache.org/r/38030/diff/
Testing
-------
No testing done until the last patch in the chain.
Thanks,
Joseph Wu
Re: Review Request 38030: [2/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/38030/#review98279
-----------------------------------------------------------
Ship it!
- 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/38030/
> -----------------------------------------------------------
>
> (Updated Sept. 9, 2015, 10:20 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone.
>
>
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add PICOJSON_USE_INT64 flag to mesos compilation flags.
>
>
> Diffs
> -----
>
> src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5
>
> Diff: https://reviews.apache.org/r/38030/diff/
>
>
> Testing
> -------
>
> No testing done until the last patch in the chain.
>
>
> Thanks,
>
> Joseph Wu
>
>