mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <bbann...@apache.org>
Subject Re: Review Request 62160: Fix stout build with newer boost versions.
Date Thu, 21 Sep 2017 08:18:16 GMT


> On Sept. 19, 2017, 9:39 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/tests/json_tests.cpp
> > Lines 358-359 (original)
> > <https://reviews.apache.org/r/62160/diff/1/?file=1817622#file1817622line358>
> >
> >     Since the code here was weird even before, it seems that we should change `JSON::True`
and `JSON::False` to be factories of `JSON::Boolean`, e.g.,
> >     
> >         Boolean False()
> >         {
> >           return {false};
> >         }
> >         
> >         Boolean True()
> >         {
> >           return {true};
> >         }
> >         
> >     With such factories we could keep users of `Boolean` as brief as they are now,
and not resort to slicing as a usual measure. This change would also be in line with what
we did when created `Bytest` factories out of existing subclasses.
> >         
> >     Would you be able to create a patch for that?
> 
> Benno Evers wrote:
>     This will break all code that creates objects of type `JSON::True` or `JSON::False`.
We could simultaneously fix all uses in mesos/libprocess, but technically `stout` is an independent
library so I think we should not break API compatibility for such a small improvement.

You are right that this breaks the stout API, but I believe this is acceptable. As for fixing
all use, you are fixing all instances where these _types_ are actually used in Mesos right
in this patch (and AFAICT there are no users of these types in libprocess or stout). All other
users use these effectively as factories for `JSON::Boolean` (possibly with intentional slicing).

In any case, I am dropping this issue as it not directly related to this patch. We can come
back to tidy up users of `JSON::Boolean(true)` or `JSON::Boolean(false)` later.


- Benjamin


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


On Sept. 12, 2017, 1:08 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62160/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2017, 1:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Starting from Boost 1.62, Boost.Variant added additional
> compile-time checks to its constructors to fix this
> issue: https://svn.boost.org/trac10/ticket/11602
> 
> However, this breaks some places in stout which try
> to access a derived class from a variant holding the
> base class.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/json.hpp 4f9ea1b34537026af80edd97fa0b3ae1a3dfa862 
>   3rdparty/stout/include/stout/protobuf.hpp 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20

>   3rdparty/stout/tests/json_tests.cpp adaa343faf3b557ad483675318b22eb90b1eeb52 
> 
> 
> Diff: https://reviews.apache.org/r/62160/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


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