mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 67632: Always define PICOJSON_USE_INT64.
Date Mon, 02 Jul 2018 09:44:43 GMT


> On July 2, 2018, 11:14 a.m., Alexander Rojas wrote:
> > 3rdparty/stout/include/stout/json.hpp
> > Lines 25 (patched)
> > <https://reviews.apache.org/r/67632/diff/3/?file=2045099#file2045099line25>
> >
> >     This breaks compilation if someone already has define `PICOJSON_USE_INT64`.
I think the cleanest way to go about it is:
> >     
> >     ```c++
> >     #ifndef PICOJSON_USE_INT64
> >     #  define PICOJSON_USE_INT64
> >     #endif
> >     ```

We cannot distinguish whether a user did not specify that flag because of us making the header
self-contained here, or due to them wanting to parameterize picojson behavior. Compiling some
picojson-dependent TUs with this flag and some without will lead to hard to diagnose issues
down the line (e.g., linker errors, ODR violation). I am against adding to much magic here
since that would make this even more intrasparent.

If your project defines this flag to link against Mesos, you do not need to set it anymore
with this change (and the "breakage" is merely your compiler giving you a heads-up and nothing
fatal). Note that Mesos provides a pkg-config file (`mesos.pc`), so there is no need to tightly
couple your project to whatever Mesos sets.

If your project defines this flag because it depends on picojson itself, please make sure
you are and remain compatible with what Mesos defines for picojson. It might make sense to
make sure that the different parameterizations of picojson never are visible at the same time
to prevent e.g., ODR violations.


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67632/#review205630
-----------------------------------------------------------


On June 26, 2018, 1:48 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67632/
> -----------------------------------------------------------
> 
> (Updated June 26, 2018, 1:48 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The macro PICOJSON_USE_INT64 must always be defined
> when attempting to use stout's JSON facilities, since
> they unconditionally depend on these features.
> 
> Previously, we relied on the Mesos build system to set
> that macro from the command line, but since we don't
> generate a separate pkg-config file for stout there is
> no way to convey this information to other users of stout.
> 
> Even for users trying to use stout as part of a libmesos
> installation, it might be surprising that it is required to
> read CPPFLAGS if only the header-only stout part is to be
> used.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/Makefile.am 5b922af6cd58dc03c44aacf88637e89f6e289702 
>   3rdparty/stout/include/stout/json.hpp 5e738cf6f72f32b852beb61a16787e48082dab14 
> 
> 
> Diff: https://reviews.apache.org/r/67632/diff/3/
> 
> 
> Testing
> -------
> 
> Installed stout on my system and compiled an external program using stout's JSON processing
functions.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message