Re: Review Request 71734: Revamped attribute handling in stout.

2019-11-07 Thread Benno Evers 

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

2019-11-06 Thread Benjamin Bannier 


> On Nov. 6, 2019, 5:03 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/exit.hpp
> > Line 65 (original), 65 (patched)
> > 
> >
> > 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.

2019-11-06 Thread Benjamin Bannier 


> On Nov. 6, 2019, 5:03 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/attributes.hpp
> > Lines 17 (patched)
> > 
> >
> > 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.

2019-11-06 Thread Benno Evers 


> On Nov. 6, 2019, 4:03 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/attributes.hpp
> > Lines 17 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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.

2019-11-06 Thread Benjamin Bannier 

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


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)


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)


nit: single line?



3rdparty/stout/include/stout/unreachable.hpp
Line 25 (original), 25 (patched)


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.

2019-11-06 Thread Benno Evers 

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