You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2016/03/24 21:23:01 UTC
Review Request 45311: Added an additional template parameter to 'Try'
for typeful error.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45311/
-----------------------------------------------------------
Review request for mesos, Alex Clemmer and Joris Van Remoortere.
Repository: mesos
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp c444c0118d39ee6a5da4618d7c62784464377280
Diff: https://reviews.apache.org/r/45311/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 45311: Added an additional template parameter to
'Try' for typeful error.
Posted by Daniel Pravat <dp...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45311/#review126797
-----------------------------------------------------------
Ship it!
Ship It!
- Daniel Pravat
On March 24, 2016, 8:23 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45311/
> -----------------------------------------------------------
>
> (Updated March 24, 2016, 8:23 p.m.)
>
>
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp c444c0118d39ee6a5da4618d7c62784464377280
>
> Diff: https://reviews.apache.org/r/45311/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 45311: Added an additional template parameter to
'Try' for typed error.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45311/
-----------------------------------------------------------
(Updated April 4, 2016, 8:25 p.m.)
Review request for mesos, Alex Clemmer and Joris Van Remoortere.
Changes
-------
Added a meaningful `static_assert` error message.
Bugs: MESOS-5110
https://issues.apache.org/jira/browse/MESOS-5110
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp c444c0118d39ee6a5da4618d7c62784464377280
Diff: https://reviews.apache.org/r/45311/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 45311: Added an additional template parameter to
'Try' for typed error.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45311/
-----------------------------------------------------------
(Updated April 4, 2016, 8:16 p.m.)
Review request for mesos, Alex Clemmer and Joris Van Remoortere.
Bugs: MESOS-5110
https://issues.apache.org/jira/browse/MESOS-5110
Repository: mesos
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp c444c0118d39ee6a5da4618d7c62784464377280
Diff: https://reviews.apache.org/r/45311/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 45311: Added an additional template parameter to
'Try' for typed error.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45311/
-----------------------------------------------------------
(Updated April 4, 2016, 8:08 p.m.)
Review request for mesos, Alex Clemmer and Joris Van Remoortere.
Summary (updated)
-----------------
Added an additional template parameter to 'Try' for typed error.
Repository: mesos
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp c444c0118d39ee6a5da4618d7c62784464377280
Diff: https://reviews.apache.org/r/45311/diff/
Testing
-------
`make check`
Thanks,
Michael Park
Re: Review Request 45311: Added an additional template parameter to
'Try' for typeful error.
Posted by Michael Park <mp...@apache.org>.
> On March 26, 2016, 8:07 p.m., Daniel Pravat wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp, line 33
> > <https://reviews.apache.org/r/45311/diff/1/?file=1314159#file1314159line33>
> >
> > With this default parameter the code has to be changed on every place where we return or handle a WindowsError object.
> >
> > One sugestion is to use different defaults on Windows and Posix and handle the conversion from ErrnoError/Error to WindowsError inside WindowsError class.
> With this default parameter the code has to be changed on every place where we return or handle a WindowsError object.
This is not true. In places that return a `Try<T>`, we only care about the error string, not the error code.
Therefore, this is backwards-compatible, non-intrusive, and no-worse-than-before, and still allows us to be more specific going forward.
> One sugestion is to use different defaults on Windows and Posix and handle the conversion from ErrnoError/Error to WindowsError inside WindowsError class.
We could take this suggestion within the `os` namespace, but not at this level.
An error code such as `errno` is only present and relevant when we interact with the C API, which is encapsulated within the `os` namespace.
Wihtin the `os` namespace we could do something like:
```cpp
namespace os {
using OsError =
#ifdef __WINDOWS__
WindowsError;
#else
ErrnoError;
#endif
template <typename T>
using Try = ::Try<T, OsError>;
} // namespace os {
```
But again, I think this is future work, and also out of the scope of this review.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45311/#review125548
-----------------------------------------------------------
On March 24, 2016, 8:23 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45311/
> -----------------------------------------------------------
>
> (Updated March 24, 2016, 8:23 p.m.)
>
>
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp c444c0118d39ee6a5da4618d7c62784464377280
>
> Diff: https://reviews.apache.org/r/45311/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 45311: Added an additional template parameter to
'Try' for typeful error.
Posted by Daniel Pravat <dp...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45311/#review125548
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp (line 33)
<https://reviews.apache.org/r/45311/#comment188403>
With this default parameter the code has to be changed on every place where we return or handle a WindowsError object.
One sugestion is to use different defaults on Windows and Posix and handle the conversion from ErrnoError/Error to WindowsError inside WindowsError class.
- Daniel Pravat
On March 24, 2016, 8:23 p.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45311/
> -----------------------------------------------------------
>
> (Updated March 24, 2016, 8:23 p.m.)
>
>
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp c444c0118d39ee6a5da4618d7c62784464377280
>
> Diff: https://reviews.apache.org/r/45311/diff/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Michael Park
>
>