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 2019/11/06 14:35:25 UTC
Review Request 71734: Revamped attribute handling in stout.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71734/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
Repository: mesos
Description
-------
This makes several changes to the attribute compatibility layer
provided by stout:
* Renamed the non-namespaced `NORETURN` macro to `STOUT_NORETURN`.
* Preferred the use of the standardized `[[noreturn]]` syntax
if supported by the compiler.
* Fixed previous usages of `NORETURN` in the stout codebase.
* Added support for the `[[nodiscard]]` attribute.
Diffs
-----
3rdparty/stout/include/stout/abort.hpp 43ed5ce2830c493e4c801cc81f8dde0922c99a8d
3rdparty/stout/include/stout/attributes.hpp aa377db82e1dbdb8727b1128780e2409accc8ae9
3rdparty/stout/include/stout/exit.hpp 34585a005063b17d0c7754c8e8c13f0641383bc4
3rdparty/stout/include/stout/unimplemented.hpp ab6caa8fa9645bca66a3efcdc6d337f3fb0481d7
3rdparty/stout/include/stout/unreachable.hpp d4b3bb0582eb9e64e6f150735d1e9f2956edbca6
Diff: https://reviews.apache.org/r/71734/diff/1/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 71734: Revamped attribute handling in stout.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71734/
-----------------------------------------------------------
(Updated Nov. 7, 2019, 2:23 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
Changes
-------
Incorporated feedback.
Repository: mesos
Description
-------
This makes several changes to the attribute compatibility layer
provided by stout:
* Add a new `STOUT_HAS_CPP_ATTRIBUTE` macro to test compiler
support for a given attribute.
* Renamed the non-namespaced `NORETURN` macro to `STOUT_NORETURN`.
* Preferred the use of the standardized `[[noreturn]]` syntax
if supported by the compiler.
* Fixed previous usages of `NORETURN` in the stout codebase.
* Added support for the `[[nodiscard]]` attribute.
Diffs (updated)
-----
3rdparty/stout/include/stout/abort.hpp 43ed5ce2830c493e4c801cc81f8dde0922c99a8d
3rdparty/stout/include/stout/attributes.hpp aa377db82e1dbdb8727b1128780e2409accc8ae9
3rdparty/stout/include/stout/exit.hpp 34585a005063b17d0c7754c8e8c13f0641383bc4
3rdparty/stout/include/stout/unimplemented.hpp ab6caa8fa9645bca66a3efcdc6d337f3fb0481d7
3rdparty/stout/include/stout/unreachable.hpp d4b3bb0582eb9e64e6f150735d1e9f2956edbca6
Diff: https://reviews.apache.org/r/71734/diff/3/
Changes: https://reviews.apache.org/r/71734/diff/2-3/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 71734: Revamped attribute handling in stout.
Posted by Benjamin Bannier <bb...@apache.org>.
> On Nov. 6, 2019, 5:03 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/exit.hpp
> > Line 65 (original), 65 (patched)
> > <https://reviews.apache.org/r/71734/diff/2/?file=2171707#file2171707line65>
> >
> > nit: single line?
>
> Benno Evers wrote:
> I thought about it, but right now (i.e. after this patch is applied) all code is inconsistent in using the
>
> ```
> attributes
> void foo(args);
> ```
> style of declaration, which looks pretty good to me.
This is mostly a matter of personal preference, but at least for such simple cases which do not come with "local consistency" precedence I'd pick whatever our `clang-format` fork would do, if only to make this less of a hassle in the future (the attribute on a separate line also has a non-C++ C vibe for me personally). So I guess the only argument I could bring would be: easier to work with since tooling-enabled.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71734/#review218544
-----------------------------------------------------------
On Nov. 6, 2019, 4:56 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71734/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2019, 4:56 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This makes several changes to the attribute compatibility layer
> provided by stout:
>
> * Add a new `STOUT_HAS_CPP_ATTRIBUTE` macro to test compiler
> support for a given attribute.
> * Renamed the non-namespaced `NORETURN` macro to `STOUT_NORETURN`.
> * Preferred the use of the standardized `[[noreturn]]` syntax
> if supported by the compiler.
> * Fixed previous usages of `NORETURN` in the stout codebase.
> * Added support for the `[[nodiscard]]` attribute.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/abort.hpp 43ed5ce2830c493e4c801cc81f8dde0922c99a8d
> 3rdparty/stout/include/stout/attributes.hpp aa377db82e1dbdb8727b1128780e2409accc8ae9
> 3rdparty/stout/include/stout/exit.hpp 34585a005063b17d0c7754c8e8c13f0641383bc4
> 3rdparty/stout/include/stout/unimplemented.hpp ab6caa8fa9645bca66a3efcdc6d337f3fb0481d7
> 3rdparty/stout/include/stout/unreachable.hpp d4b3bb0582eb9e64e6f150735d1e9f2956edbca6
>
>
> Diff: https://reviews.apache.org/r/71734/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71734: Revamped attribute handling in stout.
Posted by Benno Evers <be...@mesosphere.com>.
> On Nov. 6, 2019, 4:03 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/attributes.hpp
> > Lines 17 (patched)
> > <https://reviews.apache.org/r/71734/diff/2/?file=2171706#file2171706line17>
> >
> > Did you want to export this macro? I am not sure it would be useful elsewhere since any use of it should probably be in this file. Maybe let's just `#undef` it once we are done here.
I wasn't sure, but it seems like a macro that could be generally useful if some code wants to test for the presence of an attribute, and it's already in the correct header for attribute-related convenience methods. (and it's namespaced, so there's no need to worry about namespace pollution)
What do you think?
> On Nov. 6, 2019, 4:03 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/exit.hpp
> > Line 65 (original), 65 (patched)
> > <https://reviews.apache.org/r/71734/diff/2/?file=2171707#file2171707line65>
> >
> > nit: single line?
I thought about it, but right now (i.e. after this patch is applied) all code is inconsistent in using the
```
attributes
void foo(args);
```
style of declaration, which looks pretty good to me.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71734/#review218544
-----------------------------------------------------------
On Nov. 6, 2019, 3:56 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71734/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2019, 3:56 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This makes several changes to the attribute compatibility layer
> provided by stout:
>
> * Add a new `STOUT_HAS_CPP_ATTRIBUTE` macro to test compiler
> support for a given attribute.
> * Renamed the non-namespaced `NORETURN` macro to `STOUT_NORETURN`.
> * Preferred the use of the standardized `[[noreturn]]` syntax
> if supported by the compiler.
> * Fixed previous usages of `NORETURN` in the stout codebase.
> * Added support for the `[[nodiscard]]` attribute.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/abort.hpp 43ed5ce2830c493e4c801cc81f8dde0922c99a8d
> 3rdparty/stout/include/stout/attributes.hpp aa377db82e1dbdb8727b1128780e2409accc8ae9
> 3rdparty/stout/include/stout/exit.hpp 34585a005063b17d0c7754c8e8c13f0641383bc4
> 3rdparty/stout/include/stout/unimplemented.hpp ab6caa8fa9645bca66a3efcdc6d337f3fb0481d7
> 3rdparty/stout/include/stout/unreachable.hpp d4b3bb0582eb9e64e6f150735d1e9f2956edbca6
>
>
> Diff: https://reviews.apache.org/r/71734/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71734: Revamped attribute handling in stout.
Posted by Benjamin Bannier <bb...@apache.org>.
> On Nov. 6, 2019, 5:03 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/attributes.hpp
> > Lines 17 (patched)
> > <https://reviews.apache.org/r/71734/diff/2/?file=2171706#file2171706line17>
> >
> > Did you want to export this macro? I am not sure it would be useful elsewhere since any use of it should probably be in this file. Maybe let's just `#undef` it once we are done here.
>
> Benno Evers wrote:
> I wasn't sure, but it seems like a macro that could be generally useful if some code wants to test for the presence of an attribute, and it's already in the correct header for attribute-related convenience methods. (and it's namespaced, so there's no need to worry about namespace pollution)
>
> What do you think?
Makes sense to me.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71734/#review218544
-----------------------------------------------------------
On Nov. 6, 2019, 4:56 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71734/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2019, 4:56 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This makes several changes to the attribute compatibility layer
> provided by stout:
>
> * Add a new `STOUT_HAS_CPP_ATTRIBUTE` macro to test compiler
> support for a given attribute.
> * Renamed the non-namespaced `NORETURN` macro to `STOUT_NORETURN`.
> * Preferred the use of the standardized `[[noreturn]]` syntax
> if supported by the compiler.
> * Fixed previous usages of `NORETURN` in the stout codebase.
> * Added support for the `[[nodiscard]]` attribute.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/abort.hpp 43ed5ce2830c493e4c801cc81f8dde0922c99a8d
> 3rdparty/stout/include/stout/attributes.hpp aa377db82e1dbdb8727b1128780e2409accc8ae9
> 3rdparty/stout/include/stout/exit.hpp 34585a005063b17d0c7754c8e8c13f0641383bc4
> 3rdparty/stout/include/stout/unimplemented.hpp ab6caa8fa9645bca66a3efcdc6d337f3fb0481d7
> 3rdparty/stout/include/stout/unreachable.hpp d4b3bb0582eb9e64e6f150735d1e9f2956edbca6
>
>
> Diff: https://reviews.apache.org/r/71734/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71734: Revamped attribute handling in stout.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71734/#review218544
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/stout/include/stout/attributes.hpp
Lines 17 (patched)
<https://reviews.apache.org/r/71734/#comment306282>
Did you want to export this macro? I am not sure it would be useful elsewhere since any use of it should probably be in this file. Maybe let's just `#undef` it once we are done here.
3rdparty/stout/include/stout/attributes.hpp
Line 17 (original), 22 (patched)
<https://reviews.apache.org/r/71734/#comment306278>
Since you are add it you could add a comment here that we _always_ want to specify `noreturn` since it might enable additional optimizations (`nodiscard` OTOH likely has mostly effects in the frontend).
3rdparty/stout/include/stout/exit.hpp
Line 65 (original), 65 (patched)
<https://reviews.apache.org/r/71734/#comment306281>
nit: single line?
3rdparty/stout/include/stout/unreachable.hpp
Line 25 (original), 25 (patched)
<https://reviews.apache.org/r/71734/#comment306283>
nit: single line?
- Benjamin Bannier
On Nov. 6, 2019, 4:56 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71734/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2019, 4:56 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This makes several changes to the attribute compatibility layer
> provided by stout:
>
> * Add a new `STOUT_HAS_CPP_ATTRIBUTE` macro to test compiler
> support for a given attribute.
> * Renamed the non-namespaced `NORETURN` macro to `STOUT_NORETURN`.
> * Preferred the use of the standardized `[[noreturn]]` syntax
> if supported by the compiler.
> * Fixed previous usages of `NORETURN` in the stout codebase.
> * Added support for the `[[nodiscard]]` attribute.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/abort.hpp 43ed5ce2830c493e4c801cc81f8dde0922c99a8d
> 3rdparty/stout/include/stout/attributes.hpp aa377db82e1dbdb8727b1128780e2409accc8ae9
> 3rdparty/stout/include/stout/exit.hpp 34585a005063b17d0c7754c8e8c13f0641383bc4
> 3rdparty/stout/include/stout/unimplemented.hpp ab6caa8fa9645bca66a3efcdc6d337f3fb0481d7
> 3rdparty/stout/include/stout/unreachable.hpp d4b3bb0582eb9e64e6f150735d1e9f2956edbca6
>
>
> Diff: https://reviews.apache.org/r/71734/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71734: Revamped attribute handling in stout.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71734/
-----------------------------------------------------------
(Updated Nov. 6, 2019, 3:56 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
Changes
-------
Add STOUT_HAS_CPP_ATTRIBUTE macro.
Repository: mesos
Description (updated)
-------
This makes several changes to the attribute compatibility layer
provided by stout:
* Add a new `STOUT_HAS_CPP_ATTRIBUTE` macro to test compiler
support for a given attribute.
* Renamed the non-namespaced `NORETURN` macro to `STOUT_NORETURN`.
* Preferred the use of the standardized `[[noreturn]]` syntax
if supported by the compiler.
* Fixed previous usages of `NORETURN` in the stout codebase.
* Added support for the `[[nodiscard]]` attribute.
Diffs (updated)
-----
3rdparty/stout/include/stout/abort.hpp 43ed5ce2830c493e4c801cc81f8dde0922c99a8d
3rdparty/stout/include/stout/attributes.hpp aa377db82e1dbdb8727b1128780e2409accc8ae9
3rdparty/stout/include/stout/exit.hpp 34585a005063b17d0c7754c8e8c13f0641383bc4
3rdparty/stout/include/stout/unimplemented.hpp ab6caa8fa9645bca66a3efcdc6d337f3fb0481d7
3rdparty/stout/include/stout/unreachable.hpp d4b3bb0582eb9e64e6f150735d1e9f2956edbca6
Diff: https://reviews.apache.org/r/71734/diff/2/
Changes: https://reviews.apache.org/r/71734/diff/1-2/
Testing
-------
Thanks,
Benno Evers