> 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 > >
