You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2018/08/24 13:48:06 UTC
Review Request 68502: Added stout wrapper for
`boost::circular_buffer`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68502/
-----------------------------------------------------------
Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Till Toenshoff.
Repository: mesos
Description
-------
Added a new file `stout/circular_buffer.hpp` that pulls
the `boost::circular_buffer` class into the global namespace
to ensure the boost header is always used with the correct
macro definitions when included from Mesos.
Diffs
-----
3rdparty/stout/include/Makefile.am 0a4ea7b16b316cc8a411ac9c4d20783530b7168a
3rdparty/stout/include/stout/circular_buffer.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/68502/diff/1/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 68502: Added stout wrapper for
`boost::circular_buffer`.
Posted by Benno Evers <be...@mesosphere.com>.
> On Aug. 24, 2018, 4:50 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/circular_buffer.hpp
> > Lines 23 (patched)
> > <https://reviews.apache.org/r/68502/diff/1/?file=2077099#file2077099line23>
> >
> > As suggested in the previous patch, let's add a TODO:
> > ```
> > // TODO(bevers): Remove this once we bump the minimum Boost version.
> > ```
As replied in the previous patch, there's no plan to remove these once we bump the minimum Boost version.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68502/#review207893
-----------------------------------------------------------
On Aug. 24, 2018, 1:48 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68502/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 1:48 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new file `stout/circular_buffer.hpp` that pulls
> the `boost::circular_buffer` class into the global namespace
> to ensure the boost header is always used with the correct
> macro definitions when included from Mesos.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/Makefile.am 0a4ea7b16b316cc8a411ac9c4d20783530b7168a
> 3rdparty/stout/include/stout/circular_buffer.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68502/diff/1/
>
>
> Testing
> -------
>
> See follow-up review r/68503.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68502: Added stout wrapper for
`boost::circular_buffer`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68502/#review207893
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/stout/include/stout/circular_buffer.hpp
Lines 13 (patched)
<https://reviews.apache.org/r/68502/#comment291511>
Currently there are two headers in stout whose names contain underscores: `result_of.hpp` and `int_fd.hpp`, and both include the underscore in the macro guard. So for consistency, I suggest using `__STOUT_CIRCULAR_BUFFER_HPP__` instead.
3rdparty/stout/include/stout/circular_buffer.hpp
Lines 16-18 (patched)
<https://reviews.apache.org/r/68502/#comment291510>
These lines can be removed. See below.
3rdparty/stout/include/stout/circular_buffer.hpp
Lines 23 (patched)
<https://reviews.apache.org/r/68502/#comment291509>
As suggested in the previous patch, let's add a TODO:
```
// TODO(bevers): Remove this once we bump the minimum Boost version.
```
3rdparty/stout/include/stout/circular_buffer.hpp
Lines 27-38 (patched)
<https://reviews.apache.org/r/68502/#comment291508>
It seems to me that we don't need this. Instead, we could just do the following:
```
using boost::circular_buffer;
```
- Chun-Hung Hsiao
On Aug. 24, 2018, 1:48 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68502/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 1:48 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new file `stout/circular_buffer.hpp` that pulls
> the `boost::circular_buffer` class into the global namespace
> to ensure the boost header is always used with the correct
> macro definitions when included from Mesos.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/Makefile.am 0a4ea7b16b316cc8a411ac9c4d20783530b7168a
> 3rdparty/stout/include/stout/circular_buffer.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68502/diff/1/
>
>
> Testing
> -------
>
> See follow-up review r/68503.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68502: Added stout wrapper for
`boost::circular_buffer`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68502/#review207909
-----------------------------------------------------------
Fix it, then Ship it!
Thanks for cleaning this up Benno!
3rdparty/stout/include/Makefile.am
Line 27 (original), 27-28 (patched)
<https://reviews.apache.org/r/68502/#comment291535>
Swap the ordering of these two.
3rdparty/stout/include/stout/circular_buffer.hpp
Lines 20-21 (patched)
<https://reviews.apache.org/r/68502/#comment291536>
I was briefly wondering if these should be `undef`'ed once we have included the Boost header, but what you wrote here is actually the better solution as it would leave the chance for users with different defines seeing redefinition warnings.
- Benjamin Bannier
On Aug. 24, 2018, 7:38 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68502/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 7:38 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new file `stout/circular_buffer.hpp` that pulls
> the `boost::circular_buffer` class into the global namespace
> to ensure the boost header is always used with the correct
> macro definitions when included from Mesos.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/Makefile.am 0a4ea7b16b316cc8a411ac9c4d20783530b7168a
> 3rdparty/stout/include/stout/circular_buffer.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68502/diff/2/
>
>
> Testing
> -------
>
> See follow-up review r/68503.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68502: Added stout wrapper for
`boost::circular_buffer`.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68502/#review207916
-----------------------------------------------------------
Ship it!
Modulo Benjamin's comment. I'll fix it while committing.
- Alexander Rukletsov
On Aug. 24, 2018, 5:38 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68502/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 5:38 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Till Toenshoff.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new file `stout/circular_buffer.hpp` that pulls
> the `boost::circular_buffer` class into the global namespace
> to ensure the boost header is always used with the correct
> macro definitions when included from Mesos.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/Makefile.am 0a4ea7b16b316cc8a411ac9c4d20783530b7168a
> 3rdparty/stout/include/stout/circular_buffer.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/68502/diff/2/
>
>
> Testing
> -------
>
> See follow-up review r/68503.
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68502: Added stout wrapper for
`boost::circular_buffer`.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68502/
-----------------------------------------------------------
(Updated Aug. 24, 2018, 5:38 p.m.)
Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Till Toenshoff.
Changes
-------
Simplified code by switching to using declarations over alias templates.
Repository: mesos
Description
-------
Added a new file `stout/circular_buffer.hpp` that pulls
the `boost::circular_buffer` class into the global namespace
to ensure the boost header is always used with the correct
macro definitions when included from Mesos.
Diffs (updated)
-----
3rdparty/stout/include/Makefile.am 0a4ea7b16b316cc8a411ac9c4d20783530b7168a
3rdparty/stout/include/stout/circular_buffer.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/68502/diff/2/
Changes: https://reviews.apache.org/r/68502/diff/1-2/
Testing
-------
See follow-up review r/68503.
Thanks,
Benno Evers