[outcome] Second high level summary of review feedback accepted so far
Firstly thanks to everyone who has contributed no less than **618** emails at the time of writing to the Outcome review. Despite some on Reddit calling this review a "train wreck" and indeed some lovely things about me personally, I have found this review deeply helpful, and I believe so have most of its contributors in helping us all firm up our differing opinions about what such a vocabulary type ought to be. With regard to the previous high level summary at http://boost.2283326.n4.nabble.com/outcome-High-level-summary-of-review-feed..., I have made only these changes to the changes in that summary: 1. I have been persuaded to use longer more appropriate naming for result<T> and outcome<T>, so now the typedefed varieties with implicit conversions to their empty-capable form indicated by "=>" are: - static_checked_outcome<T> => static_checked_optional_outcome<T> static_checked_result<T> => static_checked_optional_result<T> These have narrow observers which reinterpret_cast and are therefore undefined behaviour if the state does not match, but use __builtin_unreachable() to help static analysis tooling like clang-tidy spot UB where it can be statically deduced. Conceptually, they are a very thin convenience wrap of std::variant<...>. - runtime_checked_outcome<T> => runtime_checked_optional_outcome<T> runtime_checked_result<T> => runtime_checked_optional_result<T> The runtime_checked_optional_* varieties are identical and unchanged to the outcome<T> and result<T> in the submitted library. They have wide observers with a formal empty state and it is never undefined behaviour to call any observer. This allows the programmer to save typing considerable boilerplate knowing that the runtime checks save the programming needing to spell things out. I have not been persuaded to change a single thing in their design yet apart from removing unnecessary member functions, but thanks to everybody who has tried, and if you think I am making a critical mistake, you still have a few days left to persuade me of it! For anyone not familiar with the discussion and feels that the names are excessively long, be aware they are just convenience typedefs of a more complex to configure "template<...> class outcome_impl". It is expected that end users of the library will choose the varieties they want for their code and typedef them into their local namespace as "result<T>" or whatever. Nobody is proposing that end users actually type out the full name each and every time they use them, and the documentation will make that clear. 2. I have been persuaded to replace the implicit conversion from static_checked_* varieties to runtime_checked_* varieties with explicit conversion instead as the implicit conversion could produce runtime failure where the programmer did not expect that to occur. Regarding other changes that I have accepted since the previous high level summary: 1. Outcome's STL exception throw catch to result<T> super-macro was found to be overkill given that current compilers won't replace simple throw...catch sequences with a branch, and still invoke the full throw-catch runtime machinery whatever the case. Therefore an exception to error code conversion function makes more sense than a macro. https://github.com/ned14/boost.outcome/issues/50 2. Outcome's usage of git submodules is felt by some reviewers to be a showstopper for acceptance as they could bring in non-Boost code unexpectedly in the future. I have agreed to make a cronjob script generated Boost.Outcome from standalone Outcome in the same fashion as Boost.ASIO is script generated from standalone ASIO. https://github.com/ned14/boost.outcome/issues/51 - Licensing to be BSL only - No cmake, just bjam - No git submodules at all - std-flavoured, not boost-flavoured - No code to be brought in except that written by me i.e. a hardcoded whitelist applies. - Contents of Boost repo to only contain material directly relevant to Boost. Nothing else. 3. Under the assumption that error_code_extended's use of a static global ring buffer would be controversial in this review, I hacked a quick and dirty solution expecting to have to remove it. Now we know that few disapprove, a more serious implementation will be needed. https://github.com/ned14/boost.outcome/issues/52 4. Given where Vicente appears to be heading next with expected<T, E1, E2, ...> it may be wise to make the static_checked_* varieties use std::variant<...> for storage when being compiled on C++ 17. https://github.com/ned14/boost.outcome/issues/53 Thanks, Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Tue, May 30, 2017 at 7:58 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:
- No cmake, just bjam
Wait, what? Why would we want to remove functionality that already exists? I like the CMake script since it generates a Visual Studio project file (.vcxproj). Please don't take this away, its helpful.
On 30/05/2017 16:06, Vinnie Falco via Boost wrote:
On Tue, May 30, 2017 at 7:58 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:
- No cmake, just bjam
Wait, what? Why would we want to remove functionality that already exists? I like the CMake script since it generates a Visual Studio project file (.vcxproj). Please don't take this away, its helpful.
Standalone Outcome would remain cmake based. I have no love nor use case for bjam personally. (the bigger answer is that the cmake build support is entirely implemented by the boost-lite git submodule because it's a universal build system for a distributed collection of standards aspiring libraries. The CMakeLists in the Outcome root is just a set of parameters for that universal build system. As some people consider git submodules to be a showstopper, that makes impossible cmake build support in the Boost.Outcome script generated repo because the submodule with the cmake build support must be removed) Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 5/30/17 8:06 AM, Vinnie Falco via Boost wrote:
On Tue, May 30, 2017 at 7:58 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:
- No cmake, just bjam
Wait, what? Why would we want to remove functionality that already exists? I like the CMake script since it generates a Visual Studio project file (.vcxproj). Please don't take this away, its helpful.
+1 I'm not a huge fan of CMake, but then I'm not crazy about bjam either. I think library authors should have the option of including CMake files, just as they do files for appveyor, travis, etc. Of course bjam files would remain a requirement. The question of including stuff "outside" of boost is of course subject to dispute and discussion which I guess has to be handled on a case by case basis. In this case I think it's below the minimal threshold of obtrusiveness. CMake files to add some "clutter", but those who don't use/need can ignore them. So, if the author want's to, I believe he should be able to include CMake files. Robert Ramey
On Tue, May 30, 2017 at 8:40 AM, Robert Ramey via Boost <boost@lists.boost.org> wrote:
So, if the author want's to, I believe he should be able to include CMake files
I naively assumed that the Outcome CMakeLists.txt was a straightforward, self-contained affair. After all, how complicated does it need to be for a library that provides 3 similar classes and a handful of tests? I was wrong...
I naively assumed that the Outcome CMakeLists.txt was a straightforward, self-contained affair. After all, how complicated does it need to be for a library that provides 3 similar classes and a handful of tests? I was wrong...
Such a cmake would be useless for production code. You need support for all the sanitisers and static analysis tooling; building and testing libraries in header-only, static and dynamic lib forms, C++ Module and non-module forms, and with C++ exceptions enabled or disabled; CTest scripting for Travis and Appveyor CI; CPack packaging of prebuilt binaries; documentation generation with testing of example snippet code; CDash upload of CI test outputs and build artifacts; tracking and maintenance of git submodule dependencies; permuting ABI for unstable libraries - and all that is still a subset of what the common cmake infrastructure provides for all libraries which use it. Any reasonably mature production ready cmake project ends up implementing its own edition of those sorts of features, thus duplicating work and maintenance. The common cmake infrastructure provides it for any client library based on filled in parameters. You just fill in the parameters and voila, no more work, bye bye thinking about the build or CI or test stuff when you make a new C++ library. (well, once all the many bugs are ironed out, it's still pretty immature and buggy) Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 5/30/17 10:00 AM, Niall Douglas via Boost wrote:
I naively assumed that the Outcome CMakeLists.txt was a straightforward, self-contained affair. After all, how complicated does it need to be for a library that provides 3 similar classes and a handful of tests? I was wrong...
Such a cmake would be useless for production code. You need support for all the sanitisers and static analysis tooling; building and testing libraries in header-only, static and dynamic lib forms, C++ Module and non-module forms, and with C++ exceptions enabled or disabled; CTest scripting for Travis and Appveyor CI; CPack packaging of prebuilt binaries; documentation generation with testing of example snippet code; CDash upload of CI test outputs and build artifacts; tracking and maintenance of git submodule dependencies; permuting ABI for unstable libraries - and all that is still a subset of what the common cmake infrastructure provides for all libraries which use it.
I'm not convinced it needs all that to not be "useless". I use it for generating the IDE. It does a (barely) adequate job on this, but it's better than trying to maintain the IDE project by hand.
Any reasonably mature production ready cmake project ends up implementing its own edition of those sorts of features, thus duplicating work and maintenance. The common cmake infrastructure provides it for any client library based on filled in parameters. You just fill in the parameters and voila, no more work, bye bye thinking about the build or CI or test stuff when you make a new C++ library.
This sounds like it would be a worthy project on it's own - perhaps as part of boost tools. I don't think including in the scope of a particular library is a great idea. Doing this makes reviewing the library a much larger effort for everyone involved. Robert Ramey
(well, once all the many bugs are ironed out, it's still pretty immature and buggy)
Niall
Le 30/05/2017 à 16:58, Niall Douglas via Boost a écrit :
4. Given where Vicente appears to be heading next with expected<T, E1, E2, ...> it may be wise to make the static_checked_* varieties use std::variant<...> for storage when being compiled on C++ 17. https://github.com/ned14/boost.outcome/issues/53
I'm not heading nothing. All my comments about expected<T, E1, E2, ...>where if we had in the hope you defined outcome variadic ;-) We can have C++14 (or even C++11) version of std::variant. Just someone needs to port it. It would be even better if we had also a never-empty variant. Best, Vicente
On 30/05/2017 17:01, Peter Dimov via Boost wrote:
Vicente J. Botet Escriba wrote:
We can have C++14 (or even C++11) version of std::variant. Just someone needs to port it. It would be even better if we had also a never-empty variant.
I'm on it.
The libc++ variant implementation started life as a standalone library I believe, and can be found at https://github.com/mpark/variant. Works right back to C++ 11 on all major compilers. Now that is an excellent example of external code which should be accepted into Boost without issue. Nobody can question its quality nor provenance. Boost licensed too. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
On 30/05/2017 17:01, Peter Dimov via Boost wrote:
Vicente J. Botet Escriba wrote:
We can have C++14 (or even C++11) version of std::variant. Just someone needs to port it. It would be even better if we had also a never-empty variant.
I'm on it.
The libc++ variant implementation started life as a standalone library I believe, and can be found at https://github.com/mpark/variant. Works right back to C++ 11 on all major compilers.
I know, I use it when I need an std::variant; but I wanted to implement never-valueless. Actually I wanted to implement expected<T, E...> but got a bit distracted along the way. Will get to it eventually though.
Niall Douglas wrote:
1. I have been persuaded to use longer more appropriate naming for result<T> and outcome<T>, so now the typedefed varieties with implicit conversions to their empty-capable form indicated by "=>" are:
- static_checked_outcome<T> => static_checked_optional_outcome<T> static_checked_result<T> => static_checked_optional_result<T> ... - runtime_checked_outcome<T> => runtime_checked_optional_outcome<T> runtime_checked_result<T> => runtime_checked_optional_result<T> ... Nobody is proposing that end users actually type out the full name each and every time they use them, and the documentation will make that clear.
As I already said, for me this takes away the whole point of the library, which is to provide STANDARD result types which people use in their public APIs. If everyone is typedefing their own varieties, this doesn't provide a standard infrastructure. Sure, it would be useful for people who control all of their code, but no more than that. There should be one and only one result<T>, and one and only one outcome<T>, with those names, which the world should use. Let's first see if people who favor a narrow interface are satisfied with value_if, which for me is a very good compromise. Once you have a raw pointer, things are as narrow as it gets.
On Tue, May 30, 2017 at 9:11 AM, Peter Dimov via Boost < boost@lists.boost.org> wrote:
Niall Douglas wrote:
1. I have been persuaded to use longer more appropriate naming for result<T> and outcome<T>, so now the typedefed varieties with implicit conversions to their empty-capable form indicated by "=>" are:
- static_checked_outcome<T> => static_checked_optional_outcome<T> static_checked_result<T> => static_checked_optional_result<T>
...
- runtime_checked_outcome<T> => runtime_checked_optional_outcome<T> runtime_checked_result<T> => runtime_checked_optional_result<T>
...
Nobody is proposing that end users actually type out the full name each and every time they use them, and the documentation will make that clear.
As I already said, for me this takes away the whole point of the library, which is to provide STANDARD result types which people use in their public APIs.
+1 This is not unlike function<> and shared_ptr<>, what makes those types useful at all in interfaces is the same thing that makes pointers useful in interfaces: a T * is a T * and there aren't different kinds. I don't even like having both outcome<> and result<>, if I understand correctly the reason they exist is to remove perceived inefficiencies in exception_ptr implementations. I understand that this may be important in a library motivated by perceived inefficiencies in exception handling, but it seems wrong to write a library to work around flawed language implementations, inefficiencies which clearly do not exist in the language definition.
On 30/05/2017 17:11, Peter Dimov via Boost wrote:
Niall Douglas wrote:
1. I have been persuaded to use longer more appropriate naming for result<T> and outcome<T>, so now the typedefed varieties with implicit conversions to their empty-capable form indicated by "=>" are:
- static_checked_outcome<T> => static_checked_optional_outcome<T> static_checked_result<T> => static_checked_optional_result<T> ... - runtime_checked_outcome<T> => runtime_checked_optional_outcome<T> runtime_checked_result<T> => runtime_checked_optional_result<T> ... Nobody is proposing that end users actually type out the full name each and every time they use them, and the documentation will make that clear.
As I already said, for me this takes away the whole point of the library, which is to provide STANDARD result types which people use in their public APIs.
This is not and never has been the whole point of the proposed library. Do you accept that the static checked and runtime checked varieties are orthogonal user bases? There is a camp of users who strongly prefer no runtime overhead and static checking. There is also a camp of users who strongly prefer no UB possible in an uncertainty returning object. I can see a point to both camps. Neither is obviously wrong. So seeing as Outcome's **actual** primary goal is to provide a lightweight universal error handling system which eases integration of arbitrary third party libraries each with their own preferred choice of error handling strategy, the choice to provide both options with zero cost conversion and interoperation is the obvious one to me. That's the whole point and vision of Outcome. The design of the one true result type for C++ is exclusively on Vicente and WG21 LEWG to come up with. Outcome will support whatever they decide. I appreciate that isn't what you want for Outcome Peter. You want a standard result type here and now. But that's Vicente's turf, best argue for that there. Outcome is here to grease the wheels between differences in design choice, not to declare any one right way. WG21 are better suited to do that. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
Do you accept that the static checked and runtime checked varieties are orthogonal user bases? There is a camp of users who strongly prefer no runtime overhead and static checking.
Why are their needs not served by value_if? auto r = function(); if( auto* p = r.value_if() ) { // use *p // no runtime overhead on using *p // static checkers know use of *p implies p != nullptr } Between this pattern and using `assert( r.has_value() )` directly to advise the static checker, are we not covered?
On 30/05/2017 18:43, Peter Dimov via Boost wrote:
Niall Douglas wrote:
Do you accept that the static checked and runtime checked varieties are orthogonal user bases? There is a camp of users who strongly prefer no runtime overhead and static checking.
Why are their needs not served by value_if?
auto r = function();
if( auto* p = r.value_if() ) { // use *p // no runtime overhead on using *p // static checkers know use of *p implies p != nullptr }
Between this pattern and using `assert( r.has_value() )` directly to advise the static checker, are we not covered?
I'm still pondering your idea on this for the runtime checked editions. So far I am liking it, but I need to sleep on it some more. But there was still a large minority of folk who want all-narrow observers. They haven't voiced anything to say they have changed their minds. I currently, roughly speaking, find approx 50% in favour of a runtime checked edition, approx 40% in favour of a statically checked edition. Emil and Vicente make up the 10% of folk who want something completely different. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Tue, May 30, 2017 at 10:53 AM, Niall Douglas via Boost < boost@lists.boost.org> wrote:
On 30/05/2017 18:43, Peter Dimov via Boost wrote:
Niall Douglas wrote:
Do you accept that the static checked and runtime checked varieties are orthogonal user bases? There is a camp of users who strongly prefer no runtime overhead and static checking.
Why are their needs not served by value_if?
auto r = function();
if( auto* p = r.value_if() ) { // use *p // no runtime overhead on using *p // static checkers know use of *p implies p != nullptr }
Between this pattern and using `assert( r.has_value() )` directly to advise the static checker, are we not covered?
I'm still pondering your idea on this for the runtime checked editions. So far I am liking it, but I need to sleep on it some more.
But there was still a large minority of folk who want all-narrow observers. They haven't voiced anything to say they have changed their minds.
I currently, roughly speaking, find approx 50% in favour of a runtime checked edition, approx 40% in favour of a statically checked edition.
Emil and Vicente make up the 10% of folk who want something completely different.
It is probably not a good idea to measure correctness by popular opinion, especially if the popular opinion originates in the group that won't use exceptions "because they're slow". In my experience such people will always find "Overhead!!!" in any implementation.
I currently, roughly speaking, find approx 50% in favour of a runtime checked edition, approx 40% in favour of a statically checked edition.
Emil and Vicente make up the 10% of folk who want something completely different.
It is probably not a good idea to measure correctness by popular opinion,
I think C++ Either result types will always fall into two camps, narrow and wide. As you mentioned yourself Emil, the cause is the language's current inability to specify statically correctness preconditions for function calls. That would get a lot better if something like Herb's metaclasses came along, but those are many, many years out, if ever. Until then we are trapped: either put UB on the observers so it can be trapped at compile time, or eliminate all UB so logic errors appear at runtime. You can choose either of those per function, but you cannot have both at once per function. I intend to put UB "raw" observers on the runtime checked editions, maybe using the form Peter suggested. But I am deeply opposed to having short-to-type-out observers like operator*() do UB unless the type's name loudly declares "I am an unsafe type". Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Le 30/05/2017 à 21:36, Niall Douglas via Boost a écrit :
I currently, roughly speaking, find approx 50% in favour of a runtime checked edition, approx 40% in favour of a statically checked edition.
Emil and Vicente make up the 10% of folk who want something completely different.
It is probably not a good idea to measure correctness by popular opinion, I think C++ Either result types will always fall into two camps, narrow and wide. As you mentioned yourself Emil, the cause is the language's current inability to specify statically correctness preconditions for function calls. That would get a lot better if something like Herb's metaclasses came along, but those are many, many years out, if ever.
Until then we are trapped: either put UB on the observers so it can be trapped at compile time, or eliminate all UB so logic errors appear at runtime. You can choose either of those per function, but you cannot have both at once per function. You can have two functions :)
I intend to put UB "raw" observers on the runtime checked editions, maybe using the form Peter suggested. But I am deeply opposed to having short-to-type-out observers like operator*() do UB unless the type's name loudly declares "I am an unsafe type".
:( I don't understand. Aren't we on a C++ forum? on the review of a C++ library? Do we want to banish narrow contract in this library as if this kind of access was the leprose? Do you want that boost.shared_ptr checks if not null before access on operator->? I could want that on a DEBUG version, but not on my release version. Best, Vicente
Vicente J. Botet Escriba wrote:
Do you want that boost.shared_ptr checks if not null before access on operator->?
Checking in shared_ptr::op-> is narrow, by the way, and I regret putting that in. Should have been wide. Now we have endless debates whether to drop the noexcept because LWG guideline. (A daft guideline if you ask me, but that's what happens when people don't understand what the words "undefined behavior" mean.) To expand on that a bit: T* operator->() const noexcept; Requires: get() != nullptr. Returns: get(). Narrow, a mistake. T* operator->() const noexcept; Returns: get(). Wide, as it should have been. Now of course I wouldn't widen it like T* operator->() const; Returns: get(). Remarks: If get() == nullptr, throws bad_pointer_access(). because obviously I don't want shared_ptr to introduce overhead here over a raw pointer. But outcome<T> is not a pointer and is not used as such. Throwing on value access when there's no value is actually an intended use, not something that signals a logic error. It doesn't throw logic_error, it throws the actual exception stored in the outcome by the producer. All that said, I'm actually not opposed to outcome<T>::op-> being the second above (the equivalent of value_if) and op* being effectively *operator->(). I was opposed to their existence, but if they are the price we need to pay to placate the concerns of the group wanting a narrow interface, so be it.
2017-05-30 23:49 GMT+02:00 Peter Dimov via Boost <boost@lists.boost.org>:
Vicente J. Botet Escriba wrote:
Do you want that boost.shared_ptr checks if not null before access on
operator->?
Checking in shared_ptr::op-> is narrow, by the way, and I regret putting that in. Should have been wide. Now we have endless debates whether to drop the noexcept because LWG guideline. (A daft guideline if you ask me, but that's what happens when people don't understand what the words "undefined behavior" mean.)
Peter, could you clarify the above. We can see three code snippets:
To expand on that a bit:
T* operator->() const noexcept; Requires: get() != nullptr. Returns: get().
Narrow, a mistake.
You say, you do not like the above, right? You call this mistake *only* because of this LWG guideline?
T* operator->() const noexcept; Returns: get().
Wide, as it should have been.
You say, you like the above, right? But what does it mean? That it returns a `(T*)nullptr` ? This is the same as your value_if? Regards, &rzej;
Andrzej Krzemienski wrote:
To expand on that a bit:
T* operator->() const noexcept; Requires: get() != nullptr. Returns: get().
Narrow, a mistake.
You say, you do not like the above, right? You call this mistake *only* because of this LWG guideline?
For shared_ptr, it is a mistake, period. The function does not require get() != nullptr in order to do its job, so it should not have a precondition. { return get(); } is fine.
T* operator->() const noexcept; Returns: get().
Wide, as it should have been.
You say, you like the above, right? But what does it mean? That it returns a `(T*)nullptr` ?
For shared_ptr, it just returns the pointer. For result<>, yes, it would return a nullptr when no T is present, not an invalid pointer value.
This is the same as your value_if?
Yes, as I wrote,
All that said, I'm actually not opposed to outcome<T>::op-> being the second above (the equivalent of value_if) and op* being effectively *operator->().
On 30/05/2017 22:28, Vicente J. Botet Escriba wrote:
Le 30/05/2017 à 21:36, Niall Douglas via Boost a écrit :
I intend to put UB "raw" observers on the runtime checked editions, maybe using the form Peter suggested. But I am deeply opposed to having short-to-type-out observers like operator*() do UB unless the type's name loudly declares "I am an unsafe type".
:(
I don't understand. Aren't we on a C++ forum? on the review of a C++ library?
Do we want to banish narrow contract in this library as if this kind of access was the leprose?
People have the statically checked varieties available to them if they want narrow contracts. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Le 31/05/2017 à 14:04, Niall Douglas via Boost a écrit :
On 30/05/2017 22:28, Vicente J. Botet Escriba wrote:
Le 30/05/2017 à 21:36, Niall Douglas via Boost a écrit :
I intend to put UB "raw" observers on the runtime checked editions, maybe using the form Peter suggested. But I am deeply opposed to having short-to-type-out observers like operator*() do UB unless the type's name loudly declares "I am an unsafe type".
:(
I don't understand. Aren't we on a C++ forum? on the review of a C++ library?
Do we want to banish narrow contract in this library as if this kind of access was the leprose? People have the statically checked varieties available to them if they want narrow contracts.
What if I want both? Vicente
On 31/05/2017 21:08, Vicente J. Botet Escriba wrote:
Le 31/05/2017 à 14:04, Niall Douglas via Boost a écrit :
On 30/05/2017 22:28, Vicente J. Botet Escriba wrote:
Do we want to banish narrow contract in this library as if this kind of access was the leprose? People have the statically checked varieties available to them if they want narrow contracts.
What if I want both?
You can't have a statically checked function which is always legal to call. The static analyser couldn't claim calling it to be suspicious. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Le 31/05/2017 à 22:45, Niall Douglas via Boost a écrit :
On 31/05/2017 21:08, Vicente J. Botet Escriba wrote:
Le 31/05/2017 à 14:04, Niall Douglas via Boost a écrit :
On 30/05/2017 22:28, Vicente J. Botet Escriba wrote:
Do we want to banish narrow contract in this library as if this kind of access was the leprose? People have the statically checked varieties available to them if they want narrow contracts.
What if I want both? You can't have a statically checked function which is always legal to call. The static analyser couldn't claim calling it to be suspicious.
Sorry, I'm tired to try to explain you the advantages of having both functions one with wide and and the other with narrow contract. The narrow function is not always legal to cal as it has a narrow contract. Hopping someone else could have more chance than me. Best, Vicente
2017-05-31 22:57 GMT+02:00 Vicente J. Botet Escriba via Boost < boost@lists.boost.org>:
Le 31/05/2017 à 22:45, Niall Douglas via Boost a écrit :
On 31/05/2017 21:08, Vicente J. Botet Escriba wrote:
Le 31/05/2017 à 14:04, Niall Douglas via Boost a écrit :
On 30/05/2017 22:28, Vicente J. Botet Escriba wrote:
Do we want to banish narrow contract in this library as if this kind of access was the leprose?
People have the statically checked varieties available to them if they want narrow contracts.
What if I want both?
You can't have a statically checked function which is always legal to call. The static analyser couldn't claim calling it to be suspicious.
Sorry, I'm tired to try to explain you the advantages of having both functions one with wide and and the other with narrow contract. The narrow function is not always legal to cal as it has a narrow contract.
Hopping someone else could have more chance than me.
Both narrow- and wide-contract accessors are provided in std::optional. The rationale for this is that decision whether I want "just value" or whether I want the lack of value to be treated as a run-time error is best made at the point where the value is used: not at the point where the type of the object is being decided, or at the point where object is constructed. Back in the days where `boost::optional` was being designed, some people have suggested that the fall-back value (in case optional is empty) is provided upon construction: ``` // not boost::optional: optional<int> o {fallback_value(-1)}; o = 2; assert (*o == 2); o = none; assert (*o == -1); ``` But again that it was decided that fall-back value is better determined at the call site, not at object construction site. Hence we have `value_or`. Regards, &rzej;
Le 30/05/2017 à 19:53, Niall Douglas via Boost a écrit :
On 30/05/2017 18:43, Peter Dimov via Boost wrote:
Niall Douglas wrote:
Do you accept that the static checked and runtime checked varieties are orthogonal user bases? There is a camp of users who strongly prefer no runtime overhead and static checking. Why are their needs not served by value_if?
auto r = function();
if( auto* p = r.value_if() ) { // use *p // no runtime overhead on using *p // static checkers know use of *p implies p != nullptr }
Between this pattern and using `assert( r.has_value() )` directly to advise the static checker, are we not covered? I'm still pondering your idea on this for the runtime checked editions. So far I am liking it, but I need to sleep on it some more.
But there was still a large minority of folk who want all-narrow observers. They haven't voiced anything to say they have changed their minds.
I currently, roughly speaking, find approx 50% in favour of a runtime checked edition, approx 40% in favour of a statically checked edition.
Emil and Vicente make up the 10% of folk who want something completely different.
I say that we don't have to choose. We can provide as we have already done in a lot of libraries provide wide and narrow functions. No need to split the world on two parts. No silos. value_if could be a complementary/helper interface. We don't need to provide them on all the types, but as algorithms. The implementation is the same for optional, expected, result, .... The same for T nv = value_or(pv, v); E ne = error_or(pe, e); bool b = has_error(pe, e); IMHO, most of the wide function constracts can be provided this way. There are some exceptions e.g. value that throw a different exception depending on the type, but this is because we don't have found yet the minimal interface. E.g. If we had a pe.rethrow() function, we could define value by in function of has_value, operator* and rethrow. Vicente
Le 30/05/2017 à 19:24, Niall Douglas via Boost a écrit :
On 30/05/2017 17:11, Peter Dimov via Boost wrote:
Niall Douglas wrote:
1. I have been persuaded to use longer more appropriate naming for result<T> and outcome<T>, so now the typedefed varieties with implicit conversions to their empty-capable form indicated by "=>" are:
- static_checked_outcome<T> => static_checked_optional_outcome<T> static_checked_result<T> => static_checked_optional_result<T> ... - runtime_checked_outcome<T> => runtime_checked_optional_outcome<T> runtime_checked_result<T> => runtime_checked_optional_result<T> ... Nobody is proposing that end users actually type out the full name each and every time they use them, and the documentation will make that clear. As I already said, for me this takes away the whole point of the library, which is to provide STANDARD result types which people use in their public APIs. This is not and never has been the whole point of the proposed library.
Do you accept that the static checked and runtime checked varieties are orthogonal user bases? There is a camp of users who strongly prefer no runtime overhead and static checking. There is also a camp of users who strongly prefer no UB possible in an uncertainty returning object.
I can see a point to both camps. Neither is obviously wrong. So seeing as Outcome's **actual** primary goal is to provide a lightweight universal error handling system which eases integration of arbitrary third party libraries each with their own preferred choice of error handling strategy, the choice to provide both options with zero cost conversion and interoperation is the obvious one to me. But a class can have both observer functions: narrow and wide . Why do we need two classes?
That's the whole point and vision of Outcome. The design of the one true result type for C++ is exclusively on Vicente and WG21 LEWG to come up with. Outcome will support whatever they decide. I don't understand why you say that.
I appreciate that isn't what you want for Outcome Peter. You want a standard result type here and now. But that's Vicente's turf, best argue for that there. Outcome is here to grease the wheels between differences in design choice, not to declare any one right way. WG21 are better suited to do that.
I believe Peter want a Result type independently of what the standard will provide one day, and he wants it now and not in 2020. Maybe this Result type could one day a candidate for the standard. Could you confirm, Peter? Vicente
Vicente J. Botet Escriba wrote:
I believe Peter want a Result type independently of what the standard will provide one day, and he wants it now and not in 2020. Maybe this Result type could one day a candidate for the standard. Could you confirm, Peter?
As I see things, we have a very good std::error_code infrastructure sitting (underused) in <system_error>, we have a good outline of how it's used in <filesystem>, and there we have the whole API doubled as in void api_function( ... ); // throws system_error void api_function( ..., error_code& ec ); // doesn't throw for example bool exists( const std::filesystem::path& p ); bool exists( const std::filesystem::path& p, std::error_code& ec ) noexcept; I also see that an std::result (or std::outcome, or std::result with outcome semantics as per Emil) would allow us to express that as std::result<bool> exists( const std::filesystem::path& p ) noexcept; with no apparent loss of functionality. (*) Getting people a library that has a multitude of result classes would obviously be better than nothing, but how would it move us toward the goal of having std::result? How Boost originally worked was, when there is disagreement as to how some component should work, we hammer out our differences here, produce a consensus design, produce, as a result of our process, a rationale of why we arrived at this consensus and how, get the library into the hands of our users, listen to their feedback, refine as necessary, then hand the finished product to the LWG. When some people want X, others Y, a third group Z, it's always the path of least resistance to just put X+Y+Z into the library, or a policy-based factory that can create 6^11 classes, among them X, Y, and Z. But that's really a cop-out. (*) Not quite because the straightforward implementation would throw system_error instead of filesystem_error on value() and we'll lose the path, which is an interesting angle that we need to explore.
On 30/05/2017 22:18, Peter Dimov via Boost wrote:
Getting people a library that has a multitude of result classes would obviously be better than nothing, but how would it move us toward the goal of having std::result?
How Boost originally worked was, when there is disagreement as to how some component should work, we hammer out our differences here, produce a consensus design, produce, as a result of our process, a rationale of why we arrived at this consensus and how, get the library into the hands of our users, listen to their feedback, refine as necessary, then hand the finished product to the LWG.
When some people want X, others Y, a third group Z, it's always the path of least resistance to just put X+Y+Z into the library, or a policy-based factory that can create 6^11 classes, among them X, Y, and Z. But that's really a cop-out.
You may remember back at the very beginning of this review I said that these objects were different to normal C++ design principles, there was a multitude of ways of using them and hence so many varieties of usage style in their API. I said that for most of my libraries, I decide on one single clean design and use model, but for this library I really did feel that multi-modal usage was more appropriate. Thanks to this review we have reduced down that multi-modal API into simplified categories, and hived those out into separate types so the type tells the user the semantic, all with a substantial reduction in member function count. All a definite improvement in my opinion. I don't agree that this is a cop-out. I believe that empty vs non-empty is a very obvious design improvement, it significantly improves the clarity of contract of public APIs using these objects by indicating whether an empty state can ever be returned. The wide vs narrow is much less obvious. During these last few days to put code to the problem, I put together a toy Outcome implementation using nothing but templates and std::variant<> (my very first ever C++ 17 program yay!) using: // Statically checked T|error_code template <class T> using static_checked_result = outcome<T, std::error_code, void, default_to::disabled, emptiness::never, observers::narrow>; // Runtime checked T|error_code template <class T> using runtime_checked_result = outcome<T, std::error_code, void, default_to::empty, emptiness::formal, observers::wide>; ... and tried using them in various use cases. I can find absolutely no difference in runtime overhead. So that makes the sole difference to be whether you prefer the compiler to tell you about the obvious null pointer dereference in *std::get_if(&variant) if it's clever enough, or whether you prefer some runtime default action to occur instead of dereferencing a null pointer which is an instant segfault. The former traps logic errors missed by the compiler at runtime effectively. The latter lets the programmer skip typing out boilerplate. But flipping the perspective, the former means your program may die unexpectedly in production due to some untested branch combination. The latter hides logic errors and therefore programmer bugs. I know you think this a cop out, but I think those two are unresolvable. Outcome may simply choose to implement one set and not the other out of ideological purity, but I think that a "cut off one's nose to spite one's face" type of decision. It's a different form of cop out: we can't resolve this, so we choose one and screw the other.
(*) Not quite because the straightforward implementation would throw system_error instead of filesystem_error on value() and we'll lose the path, which is an interesting angle that we need to explore.
What we really actually need is an error_code which can carry payload. Lawrence's status_code is heading the right way, but I am not keen on his formulation. I have a gut feeling that we can do better, though avoiding malloc and keeping the object useful is hard. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Le 30/05/2017 à 23:18, Peter Dimov via Boost a écrit :
Vicente J. Botet Escriba wrote:
I believe Peter want a Result type independently of what the standard will provide one day, and he wants it now and not in 2020. Maybe this Result type could one day a candidate for the standard. Could you confirm, Peter?
As I see things, we have a very good std::error_code infrastructure sitting (underused) in <system_error>, we have a good outline of how it's used in <filesystem>, and there we have the whole API doubled as in
void api_function( ... ); // throws system_error void api_function( ..., error_code& ec ); // doesn't throw
for example
bool exists( const std::filesystem::path& p ); bool exists( const std::filesystem::path& p, std::error_code& ec ) noexcept;
I also see that an std::result (or std::outcome, or std::result with outcome semantics as per Emil) would allow us to express that as
std::result<bool> exists( const std::filesystem::path& p ) noexcept;
with no apparent loss of functionality. (*) We have yet some trouble with constructors. We need factories instead :(
Getting people a library that has a multitude of result classes would obviously be better than nothing, but how would it move us toward the goal of having std::result?
How Boost originally worked was, when there is disagreement as to how some component should work, we hammer out our differences here, produce a consensus design, produce, as a result of our process, a rationale of why we arrived at this consensus and how, get the library into the hands of our users, listen to their feedback, refine as necessary, then hand the finished product to the LWG.
When some people want X, others Y, a third group Z, it's always the path of least resistance to just put X+Y+Z into the library, or a policy-based factory that can create 6^11 classes, among them X, Y, and Z. But that's really a cop-out.
(*) Not quite because the straightforward implementation would throw system_error instead of filesystem_error on value() and we'll lose the path, which is an interesting angle that we need to explore. If we want to be able to throw a specific exception, we need to ensure a single mapping from E to Exception. We can do it in two ways
Wrap E so that the conversion to Exception can be done, e.g. instead of std::result<bool> we could have expected<bool, filesystem::error_code> filesystem::error_code is a wrapper for error_code that defines the rethrow function throwing filesystem_error. Again the problem is that now we need to unwrap filesystem::error_code. Vicente
Vicente J. Botet Escriba wrote: ...
Wrap E so that the conversion to Exception can be done, e.g. instead of std::result<bool> we could have expected<bool, filesystem::error_code>
filesystem::error_code is a wrapper for error_code that defines the rethrow function throwing filesystem_error.
The problem here is not just that we have to throw filesystem_error; the problem is that the point of throwing filesystem_error instead of system_error is that it contains the path (two paths even, although we only have one in this case), and there's no path in error_code. If all we had was error_code, the right exception to throw is system_error, because that's what it's for. The correct approach here, I think, is not to define fat error codes, but to return an outcome whose error() is the error_code and whose exception() is the appropriate filesystem_error exception, complete with the path(s). Setting however both the error_code and the exception_ptr is not supported by the current outcome design (and neither would it be supported by expected<R, error_code, exception_ptr>.) Or, an alternative that scales better would be to take http://pdimov.com/cpp2/P0640R0.pdf and store error_code and exception_info, so that later one can use throw_with_info( std::system_error( error() ), info ); with the additional information (such as paths) being carried by the exception_info.
The correct approach here, I think, is not to define fat error codes, but to return an outcome whose error() is the error_code and whose exception() is the appropriate filesystem_error exception, complete with the path(s).
Setting however both the error_code and the exception_ptr is not supported by the current outcome design (and neither would it be supported by expected<R, error_code, exception_ptr>.)
Or, an alternative that scales better would be to take
http://pdimov.com/cpp2/P0640R0.pdf
and store error_code and exception_info, so that later one can use
throw_with_info( std::system_error( error() ), info );
with the additional information (such as paths) being carried by the exception_info.
In AFIO v2 I'd personally consider any potential memory allocation in any of the core API unacceptable (because we never allocate memory in AFIO v2). Currently we memcpy() the tail of the path into the error_code_extended on failure, but even that I dislike because each afio::io_handle already knows its own path, and if it weren't for lifetime issues I'd just return the pointer to that path and save a lot of work for everybody. Down the line AFIO v2 will gain a "fat path" type which is the combination of an open handle with a path fragment. Those necessarily will need to decide what to do about lifetime issues. I may yet go light and make the fat path type just a naked pointer plus filesystem::path, or I may implement a process-wide registry of "path handles" i.e. handles opened solely to indicate some unknown location on the filesystem which is a base for path fragments to work from. I haven't decided yet, there are a lot of competing tradeoffs, I specifically want the end user to do all their malloc-work in a pre-init step and away from any core i/o loops which need to never, ever allocate memory. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Le 05/06/2017 à 01:08, Peter Dimov via Boost a écrit :
Vicente J. Botet Escriba wrote: ...
Wrap E so that the conversion to Exception can be done, e.g. instead of std::result<bool> we could have expected<bool, filesystem::error_code>
filesystem::error_code is a wrapper for error_code that defines the rethrow function throwing filesystem_error.
The problem here is not just that we have to throw filesystem_error; the problem is that the point of throwing filesystem_error instead of system_error is that it contains the path (two paths even, although we only have one in this case), and there's no path in error_code.
If all we had was error_code, the right exception to throw is system_error, because that's what it's for.
The correct approach here, I think, is not to define fat error codes, but to return an outcome whose error() is the error_code and whose exception() is the appropriate filesystem_error exception, complete with the path(s).
Setting however both the error_code and the exception_ptr is not supported by the current outcome design (and neither would it be supported by expected<R, error_code, exception_ptr>.)
Or, an alternative that scales better would be to take
http://pdimov.com/cpp2/P0640R0.pdf I'll take a look later.
and store error_code and exception_info, so that later one can use
throw_with_info( std::system_error( error() ), info );
with the additional information (such as paths) being carried by the exception_info. Oh, in this case it is clear that result<T> is not enough. We need expected<T, filesystem::error>
where filesystem:error contains the error_code and the path. Vicente
Vicente J. Botet Escriba wrote:
Oh, in this case it is clear that result<T> is not enough. We need expected<T, filesystem::error>
where filesystem:error contains the error_code and the path.
I don't find this particularly appealing, because the whole point of std::error_code was to avoid the need for each library to define separate error classes. expected<T, filesystem::error> will work, I'd just not be willing to recommend it as a general solution. Something like Niall's new outcome<T> that stores both an error_code and an exception seems better, because you can use a single common return type in all libraries.
Le 05/06/2017 à 16:46, Peter Dimov via Boost a écrit :
Vicente J. Botet Escriba wrote:
Oh, in this case it is clear that result<T> is not enough. We need expected<T, filesystem::error>
where filesystem:error contains the error_code and the path.
I don't find this particularly appealing, because the whole point of std::error_code was to avoid the need for each library to define separate error classes. expected<T, filesystem::error> will work, I'd just not be willing to recommend it as a general solution.
Something like Niall's new outcome<T> that stores both an error_code and an exception seems better, because you can use a single common return type in all libraries. Let define filesystem::error as we consider is the more pertinent. You don't need a specific expected/outcome/xxxxx class to cover this case. Do you?
Vicente
On 5/06/2017 11:08, Peter Dimov wrote:
The correct approach here, I think, is not to define fat error codes, but to return an outcome whose error() is the error_code and whose exception() is the appropriate filesystem_error exception, complete with the path(s).
This sounds like the design I was suggesting in the "to variant or not to variant" thread. On 6/06/2017 02:46, Peter Dimov wrote:
Vicente J. Botet Escriba wrote:
Oh, in this case it is clear that result<T> is not enough. We need expected<T, filesystem::error> where filesystem:error contains the error_code and the path.
I don't find this particularly appealing, because the whole point of std::error_code was to avoid the need for each library to define separate error classes. expected<T, filesystem::error> will work, I'd just not be willing to recommend it as a general solution.
Is it more attractive if perhaps we had a result<T, E> where E was constrained to a type derived from std::error_code? (I'm imagining a sort of orthogonal hierarchy with types derived from std::exception, although not for error-identification purposes, just to carry additional data payloads.) Implicit slicing would allow this to be used generically in contexts where an error_code is expected. The error_code design is a little unfortunate since users are encouraged to pass them around by value, which would slice off additional context information. OTOH even if passed around by reference so the information is preserved, it wouldn't be readily accessible without dynamic_casts. (The same is true for exceptions as well, but some language/compiler conspiracy hides this.) The idea of error_code having an arbitrary error-info pointer seems attractive, but the above semantics would probably require it being a shared_ptr<void>, which would probably annoy everybody for various reasons (including type safety, atomics, and memory allocation). Niall's error_code_extended is a sort of half-way point where it provides some additional fixed data payload which is probably useful in many cases as a compromise type; additionally defined as POD to avoid dynamic allocation and so that the ring buffer stomping over the data doesn't upset too many things. It's a good idea, and I'm not sure if we can come up with something better (other than arguing about what members it should have), but perhaps there are some other possibilities to consider. Perhaps rather than allowing arbitrary Es we could have a single E that still provides some additional flexibility: template<typename EI> struct error_code_info : public error_code { // ... const optional<EI>& error_info() const; // ... optional<EI> m_error_info; }; template<typename T, typename EI> using result = expected_impl<T, error_code_info<EI>>; (Again, just a sketch to present an idea; don't get too hung up on the specifics.) Could something like this work? An error can be specified with an arbitrary data payload, and yet consumers could just slice that off and treat it as a plain error_code if they want. Meanwhile the result/expected implementation can rely on noexcept/move guarantees provided by error_code_info. The main complication I see with this is that you probably don't want to over-constrain the EI type, to simplify usage -- that's why I suggested it store optional<EI> rather than EI directly, so that error_code_info could be noexcept in all cases; if EI doesn't have noexcept move/copy then error_code_info just discards the info if EI's constructor throws. (So this does allow an empty state and the empty state should not be considered weird; consumer code just needs to tolerate that like they would any other optional<>.) This does mean that consumers can introduce memory allocation into their error codes (probably via std::string), which some people won't like -- but this leaves the choice of doing that or avoiding that up to them (and the libraries they choose to consume). The main downside I see of this is that it's less straightforward to pass around error_code_info<EI>s than error_codes, but since this should be mostly confined to a well defined call chain (with specific concrete EI values rather than generics) I hope this wouldn't be a problem in actual practice. (Using a shared_ptr<void> would avoid that issue but I think that's probably worse overall.) It might also result in a proliferation of EI types (as each method of each library could conceivably define a unique one) but again I doubt that should be an issue in practice.
2017-06-06 4:35 GMT+02:00 Gavin Lambert via Boost <boost@lists.boost.org>:
On 5/06/2017 11:08, Peter Dimov wrote:
The correct approach here, I think, is not to define fat error codes, but to return an outcome whose error() is the error_code and whose exception() is the appropriate filesystem_error exception, complete with the path(s).
This sounds like the design I was suggesting in the "to variant or not to variant" thread.
On 6/06/2017 02:46, Peter Dimov wrote:
Vicente J. Botet Escriba wrote:
Oh, in this case it is clear that result<T> is not enough. We need
expected<T, filesystem::error> where filesystem:error contains the error_code and the path.
I don't find this particularly appealing, because the whole point of std::error_code was to avoid the need for each library to define separate error classes. expected<T, filesystem::error> will work, I'd just not be willing to recommend it as a general solution.
Is it more attractive if perhaps we had a result<T, E> where E was constrained to a type derived from std::error_code? (I'm imagining a sort of orthogonal hierarchy with types derived from std::exception, although not for error-identification purposes, just to carry additional data payloads.) Implicit slicing would allow this to be used generically in contexts where an error_code is expected.
The error_code design is a little unfortunate since users are encouraged to pass them around by value, which would slice off additional context information. OTOH even if passed around by reference so the information is preserved, it wouldn't be readily accessible without dynamic_casts. (The same is true for exceptions as well, but some language/compiler conspiracy hides this.)
The idea of error_code having an arbitrary error-info pointer seems attractive, but the above semantics would probably require it being a shared_ptr<void>, which would probably annoy everybody for various reasons (including type safety, atomics, and memory allocation).
Niall's error_code_extended is a sort of half-way point where it provides some additional fixed data payload which is probably useful in many cases as a compromise type; additionally defined as POD to avoid dynamic allocation and so that the ring buffer stomping over the data doesn't upset too many things. It's a good idea, and I'm not sure if we can come up with something better (other than arguing about what members it should have), but perhaps there are some other possibilities to consider.
Perhaps rather than allowing arbitrary Es we could have a single E that still provides some additional flexibility:
template<typename EI> struct error_code_info : public error_code { // ... const optional<EI>& error_info() const; // ... optional<EI> m_error_info; };
template<typename T, typename EI> using result = expected_impl<T, error_code_info<EI>>;
(Again, just a sketch to present an idea; don't get too hung up on the specifics.)
Could something like this work? An error can be specified with an arbitrary data payload, and yet consumers could just slice that off and treat it as a plain error_code if they want. Meanwhile the result/expected implementation can rely on noexcept/move guarantees provided by error_code_info.
The main complication I see with this is that you probably don't want to over-constrain the EI type, to simplify usage -- that's why I suggested it store optional<EI> rather than EI directly, so that error_code_info could be noexcept in all cases; if EI doesn't have noexcept move/copy then error_code_info just discards the info if EI's constructor throws.
(So this does allow an empty state and the empty state should not be considered weird; consumer code just needs to tolerate that like they would any other optional<>.)
This does mean that consumers can introduce memory allocation into their error codes (probably via std::string), which some people won't like -- but this leaves the choice of doing that or avoiding that up to them (and the libraries they choose to consume).
The main downside I see of this is that it's less straightforward to pass around error_code_info<EI>s than error_codes, but since this should be mostly confined to a well defined call chain (with specific concrete EI values rather than generics) I hope this wouldn't be a problem in actual practice. (Using a shared_ptr<void> would avoid that issue but I think that's probably worse overall.)
It might also result in a proliferation of EI types (as each method of each library could conceivably define a unique one) but again I doubt that should be an issue in practice.
But does this preserve the guarantee that result<T, E> is trivially copyable? Another alternative to consider is to change something in the ring buffer records, so that it resembles: ``` struct RingBufferRecord { enum {/*name different kind of payloads*/} _discriminator; union { /*define different kinds of payload*/ } }; ``` This way the enumeration tells you how to reinterpret the data. You do not know all possible payloads when designing the ring buffer, so you probably need to introduce something akin to virtual functions: ``` struct RingBufferRecord { aligned_storage<256> _raw_storage; void (*_interpret_raw_storage) (aligned_storage<256>&); }; ``` I admit this idea is not well thought over. Regards, &rzej;
But does this preserve the guarantee that result<T, E> is trivially copyable?
Also, if you want the TRY operation to work (and you really, really do, it is an enormous boilerplate saver), then your error type needs to be identical throughout your program. Templating it breaks that.
Another alternative to consider is to change something in the ring buffer records, so that it resembles:
``` struct RingBufferRecord { enum {/*name different kind of payloads*/} _discriminator; union { /*define different kinds of payload*/ } }; ```
This way the enumeration tells you how to reinterpret the data. You do not know all possible payloads when designing the ring buffer, so you probably need to introduce something akin to virtual functions:
``` struct RingBufferRecord { aligned_storage<256> _raw_storage; void (*_interpret_raw_storage) (aligned_storage<256>&); }; ```
I admit this idea is not well thought over.
You can probably set a custom ring buffer design per process, but as it would cause error_code_extended's type signature to change in order to prevent ABI collision, going finer grained than that probably wouldn't be practical as again you'd lose the TRY operation. I am currently minded to have error_code_extended no longer store a 191 byte string which lets me pack many more of them into a reasonably sized ringbuffer. If people want to store a string, I am minded they should be either supplying a static const char * or else return an outcome<T> with shared_ptr payload to strings. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 6/06/2017 21:23, Niall Douglas wrote:
Also, if you want the TRY operation to work (and you really, really do, it is an enormous boilerplate saver), then your error type needs to be identical throughout your program. Templating it breaks that.
Why does it need to be identical? Can't you just use decltype(r.error())? As long as you know that at minimum it supports the error_code interface this should be sufficient for most purposes.
I am currently minded to have error_code_extended no longer store a 191 byte string which lets me pack many more of them into a reasonably sized ringbuffer. If people want to store a string, I am minded they should be either supplying a static const char * or else return an outcome<T> with shared_ptr payload to strings.
How would they do such a payload for an error case?
On 07/06/2017 00:02, Gavin Lambert via Boost wrote:
On 6/06/2017 21:23, Niall Douglas wrote:
Also, if you want the TRY operation to work (and you really, really do, it is an enormous boilerplate saver), then your error type needs to be identical throughout your program. Templating it breaks that.
Why does it need to be identical? Can't you just use decltype(r.error())? As long as you know that at minimum it supports the error_code interface this should be sufficient for most purposes.
It doesn't work unfortunately because you need to convert from error_code interface compliant type A to error_code interface compliant type B, and what does "convert" mean exactly? Remember, the TRY implementation can have no knowledge of what the return type of the enclosing function is. It has to return something, safely, which will propagate the error code correctly. Sure, the programmer can build in conversion routines between error code interface types, we could even use a free function based design so arbitrary third party controlled types can be converted from and to. But to be honest at this stage of complexity you're better off using exceptions.
I am currently minded to have error_code_extended no longer store a 191 byte string which lets me pack many more of them into a reasonably sized ringbuffer. If people want to store a string, I am minded they should be either supplying a static const char * or else return an outcome<T> with shared_ptr payload to strings.
How would they do such a payload for an error case?
For result<T>, your payload will probably be restricted to two 64 bit pointers. Up to you what those mean (and how you'd garbage collect them). I may allow people to create a secondary ring buffer and implement some sort of chaining facility, that way they could set up a fixed sized string facility if they wanted to. For outcome<T>, use the shared_ptr facility to deliver arbitrary type-erased payload. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Wed, Jun 7, 2017 at 6:43 AM, Niall Douglas via Boost <boost@lists.boost.org> wrote:
On 07/06/2017 00:02, Gavin Lambert via Boost wrote:
On 6/06/2017 21:23, Niall Douglas wrote:
Also, if you want the TRY operation to work (and you really, really do, it is an enormous boilerplate saver), then your error type needs to be identical throughout your program. Templating it breaks that.
Why does it need to be identical? Can't you just use decltype(r.error())? As long as you know that at minimum it supports the error_code interface this should be sufficient for most purposes.
It doesn't work unfortunately because you need to convert from error_code interface compliant type A to error_code interface compliant type B, and what does "convert" mean exactly? Remember, the TRY implementation can have no knowledge of what the return type of the enclosing function is.
There was talk (not sure if it was an official proposal or just a suggestion) of something like decltype(return) for getting the type returned from the current function.
It has to return something, safely, which will propagate the error code correctly.
Sure, the programmer can build in conversion routines between error code interface types, we could even use a free function based design so arbitrary third party controlled types can be converted from and to. But to be honest at this stage of complexity you're better off using exceptions.
I am currently minded to have error_code_extended no longer store a 191 byte string which lets me pack many more of them into a reasonably sized ringbuffer. If people want to store a string, I am minded they should be either supplying a static const char * or else return an outcome<T> with shared_ptr payload to strings.
How would they do such a payload for an error case?
For result<T>, your payload will probably be restricted to two 64 bit pointers. Up to you what those mean (and how you'd garbage collect them). I may allow people to create a secondary ring buffer and implement some sort of chaining facility, that way they could set up a fixed sized string facility if they wanted to.
For outcome<T>, use the shared_ptr facility to deliver arbitrary type-erased payload.
Niall
-- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 8/06/2017 03:02, Gottlob Frege wrote:
On Wed, Jun 7, 2017 at 6:43 AM, Niall Douglas wrote:
On 07/06/2017 00:02, Gavin Lambert wrote:
On 6/06/2017 21:23, Niall Douglas wrote:
Also, if you want the TRY operation to work (and you really, really do, it is an enormous boilerplate saver), then your error type needs to be identical throughout your program. Templating it breaks that.
Why does it need to be identical? Can't you just use decltype(r.error())? As long as you know that at minimum it supports the error_code interface this should be sufficient for most purposes.
It doesn't work unfortunately because you need to convert from error_code interface compliant type A to error_code interface compliant type B, and what does "convert" mean exactly? Remember, the TRY implementation can have no knowledge of what the return type of the enclosing function is.
There was talk (not sure if it was an official proposal or just a suggestion) of something like decltype(return) for getting the type returned from the current function.
Even without that, couldn't TRY accept the return type as a parameter? Or perhaps rather than returning directly it could output to a parameter that the enclosing code declares to be the same as the return type (especially since you say you're using this two-phase in many cases anyway). Besides, if the intent is to convey the payload up the call chain, then it can be left to the responsibility of the method itself to forward that appropriately -- ie. if you call something that can return an error_code_info<EI1> then you are required to do one of the following: 1. Deal with the error locally, not calling TRY. 2. Call TRY to pass on the error unmodified, thereby requiring that you also return an error_code_info<EI1>. 3. Pass on the error, but returning error_code_info<EI2> where EI2 is constructible from EI1 (forwarding the original payload plus some additional context). This might be a special case of #1. (By "returning" here I don't mean directly, I mean wrapped in a result<> or outcome<>.)
Why does it need to be identical? Can't you just use decltype(r.error())? As long as you know that at minimum it supports the error_code interface this should be sufficient for most purposes.
It doesn't work unfortunately because you need to convert from error_code interface compliant type A to error_code interface compliant type B, and what does "convert" mean exactly? Remember, the TRY implementation can have no knowledge of what the return type of the enclosing function is.
There was talk (not sure if it was an official proposal or just a suggestion) of something like decltype(return) for getting the type returned from the current function.
Even without that, couldn't TRY accept the return type as a parameter?
It doesn't especially help you in practice. What Outcome currently does is return magic type sugar from TRY which is implicitly constructible by any basic_monad instance, this works around needing to know what the return type is for the calling function in the TRY implementation.
Besides, if the intent is to convey the payload up the call chain, then it can be left to the responsibility of the method itself to forward that appropriately -- ie. if you call something that can return an error_code_info<EI1> then you are required to do one of the following:
1. Deal with the error locally, not calling TRY. 2. Call TRY to pass on the error unmodified, thereby requiring that you also return an error_code_info<EI1>. 3. Pass on the error, but returning error_code_info<EI2> where EI2 is constructible from EI1 (forwarding the original payload plus some additional context). This might be a special case of #1.
(By "returning" here I don't mean directly, I mean wrapped in a result<> or outcome<>.)
Of course you could do all that. What I'm saying is that for 95% of programmers, when facing the choice of whether to use different E types in exchange for typing a ton load more boilerplate all day long every day in everything they write, they will - I guarantee you - choose just to make the error type the same throughout the entire program. It makes things *simple*. All that said, given that my non-variant Outcome plan has much more compile time budget than before, I certainly can make constructors all templated and SFINAEd in a way presented Outcome could not without blowing out compile times. One therefore could enable implicit construction from some arbitrary other error type if the user has implemented implicit conversion between error types. I think that would enable what you are proposing? Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 8/06/2017 23:36, Niall Douglas wrote:
It doesn't especially help you in practice. What Outcome currently does is return magic type sugar from TRY which is implicitly constructible by any basic_monad instance, this works around needing to know what the return type is for the calling function in the TRY implementation.
Fair enough. That could still work for arbitrary EI types though -- you automatically know what the type used by the method you're calling is, and you can assume that the caller either returns the same thing or something compatible with it, as I said below. Otherwise it will be a compiler error, and rightly so.
Besides, if the intent is to convey the payload up the call chain, then it can be left to the responsibility of the method itself to forward that appropriately -- ie. if you call something that can return an error_code_info<EI1> then you are required to do one of the following:
1. Deal with the error locally, not calling TRY. 2. Call TRY to pass on the error unmodified, thereby requiring that you also return an error_code_info<EI1>. 3. Pass on the error, but returning error_code_info<EI2> where EI2 is constructible from EI1 (forwarding the original payload plus some additional context). This might be a special case of #1.
(By "returning" here I don't mean directly, I mean wrapped in a result<> or outcome<>.)
Of course you could do all that.
What I'm saying is that for 95% of programmers, when facing the choice of whether to use different E types in exchange for typing a ton load more boilerplate all day long every day in everything they write, they will - I guarantee you - choose just to make the error type the same throughout the entire program. It makes things *simple*.
People don't seem to have a problem with multiple exception classes carrying different payloads all derived from std::exception. This is an orthogonal idea, albeit simplifying it with only a single templated derived type. (I'm not opposed to having separate derived types either, but I think the templated design is better since you can provide better guarantees to the outcome implementation.) I don't think it'd be as hard to grasp as you're making out. If we take Filesystem as an example, it's likely that most methods would return an error_code_info<filename_info> that contains the filename affected by the error (which you might argue is useless to the caller since it already knows, but again imagine this being bubbled several levels up the call stack to code that didn't know the filename any other way). A few might return a plain error_code because they didn't have a filename to work on. Something like copy_file might return a type derived from filename_info that adds a second filename. That sort of thing. Or maybe Filesystem itself would only return plain error_codes, but then the caller (who constructed a filename unknown to the grandparent) might be the one that wants to include the actual filename when passing on the error, since it knows that its caller has no other way to know it.
On 06/07/2017 05:02 PM, Gottlob Frege via Boost wrote:
There was talk (not sure if it was an official proposal or just a suggestion) of something like decltype(return) for getting the type returned from the current function.
It was part of the P0536R0 "Implicit Return Type..." proposal, but it was rejected. The reason can be found here: https://botondballo.wordpress.com/2017/03/27/trip-report-c-standards-meeting...
On 05/30/2017 11:18 PM, Peter Dimov via Boost wrote:
As I see things, we have a very good std::error_code infrastructure sitting (underused) in <system_error>, we have a good outline of how it's used in <filesystem>, and there we have the whole API doubled as in
void api_function( ... ); // throws system_error void api_function( ..., error_code& ec ); // doesn't throw
for example
bool exists( const std::filesystem::path& p ); bool exists( const std::filesystem::path& p, std::error_code& ec ) noexcept;
Which reminds me, does anybody know why boost::system::throws was not used to unite the two functions as outlined in N2838 [1]? Was it the inability to specify noexcept? [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2009/n2838.html
2017-05-30 18:11 GMT+02:00 Peter Dimov via Boost <boost@lists.boost.org>:
Niall Douglas wrote:
1. I have been persuaded to use longer more appropriate naming for result<T> and outcome<T>, so now the typedefed varieties with implicit conversions to their empty-capable form indicated by "=>" are:
- static_checked_outcome<T> => static_checked_optional_outcome<T> static_checked_result<T> => static_checked_optional_result<T>
...
- runtime_checked_outcome<T> => runtime_checked_optional_outcome<T> runtime_checked_result<T> => runtime_checked_optional_result<T>
...
Nobody is proposing that end users actually type out the full name each and every time they use them, and the documentation will make that clear.
As I already said, for me this takes away the whole point of the library, which is to provide STANDARD result types which people use in their public APIs.
If everyone is typedefing their own varieties, this doesn't provide a standard infrastructure. Sure, it would be useful for people who control all of their code, but no more than that.
There should be one and only one result<T>, and one and only one outcome<T>, with those names, which the world should use.
I support this. I like the initial choice of this library. What percentage does this make of people wanting one outcome and one result?
Let's first see if people who favor a narrow interface are satisfied with value_if, which for me is a very good compromise. Once you have a raw pointer, things are as narrow as it gets.
I'll respond to this separately. Regards, &rzej;
Le 30/05/2017 à 23:28, Andrzej Krzemienski via Boost a écrit :
2017-05-30 18:11 GMT+02:00 Peter Dimov via Boost <boost@lists.boost.org>:
Let's first see if people who favor a narrow interface are satisfied with value_if, which for me is a very good compromise. Once you have a raw pointer, things are as narrow as it gets.
Sorry Peter I don't receive your mails.
value_if has a wide contract. I don't have nothing agains it. value_if It can be constructed from has_value and operator* (call it deref if you prefer). So the minimal interface is not value_if. When I kno wthat I have a value in I don't want to use value_if and dereference the obtained pointer. Vicente
2017-05-30 16:58 GMT+02:00 Niall Douglas via Boost <boost@lists.boost.org>:
Firstly thanks to everyone who has contributed no less than **618** emails at the time of writing to the Outcome review. Despite some on Reddit calling this review a "train wreck" and indeed some lovely things about me personally, I have found this review deeply helpful, and I believe so have most of its contributors in helping us all firm up our differing opinions about what such a vocabulary type ought to be.
With regard to the previous high level summary at http://boost.2283326.n4.nabble.com/outcome-High-level- summary-of-review-feedback-accepted-so-far-tp4694767.html, I have made only these changes to the changes in that summary:
Thanks Niall, for the summary. But now, after these changes, the review I am still writing is useless, as it applies to a different library. Maybe we should schedule another review?
1. I have been persuaded to use longer more appropriate naming for result<T> and outcome<T>, so now the typedefed varieties with implicit conversions to their empty-capable form indicated by "=>" are:
- static_checked_outcome<T> => static_checked_optional_outcome<T> static_checked_result<T> => static_checked_optional_result<T>
No, no. I strongly suggest to provide only one outcome, with empty state if needed, and both narrow and wide contracts. We have learned to live with such types in C++, even if they are far from perfect.
These have narrow observers which reinterpret_cast and are therefore undefined behaviour if the state does not match, but use __builtin_unreachable() to help static analysis tooling like clang-tidy spot UB where it can be statically deduced. Conceptually, they are a very thin convenience wrap of std::variant<...>.
- runtime_checked_outcome<T> => runtime_checked_optional_outcome<T> runtime_checked_result<T> => runtime_checked_optional_result<T>
The runtime_checked_optional_* varieties are identical and unchanged to the outcome<T> and result<T> in the submitted library. They have wide observers with a formal empty state and it is never undefined behaviour to call any observer. This allows the programmer to save typing considerable boilerplate knowing that the runtime checks save the programming needing to spell things out. I have not been persuaded to change a single thing in their design yet apart from removing unnecessary member functions, but thanks to everybody who has tried, and if you think I am making a critical mistake, you still have a few days left to persuade me of it!
For anyone not familiar with the discussion and feels that the names are excessively long, be aware they are just convenience typedefs of a more complex to configure "template<...> class outcome_impl". It is expected that end users of the library will choose the varieties they want for their code and typedef them into their local namespace as "result<T>" or whatever. Nobody is proposing that end users actually type out the full name each and every time they use them, and the documentation will make that clear.
Regarding other changes that I have accepted since the previous high level summary:
2. Outcome's usage of git submodules is felt by some reviewers to be a showstopper for acceptance as they could bring in non-Boost code unexpectedly in the future. I have agreed to make a cronjob script generated Boost.Outcome from standalone Outcome in the same fashion as Boost.ASIO is script generated from standalone ASIO. https://github.com/ned14/boost.outcome/issues/51
- Licensing to be BSL only - No cmake, just bjam - No git submodules at all - std-flavoured, not boost-flavoured - No code to be brought in except that written by me i.e. a hardcoded whitelist applies. - Contents of Boost repo to only contain material directly relevant to Boost. Nothing else.
Thanks. This will make the acceptance easier.
3. Under the assumption that error_code_extended's use of a static global ring buffer would be controversial in this review, I hacked a quick and dirty solution expecting to have to remove it. Now we know that few disapprove, a more serious implementation will be needed. https://github.com/ned14/boost.outcome/issues/52
I am not sure of that. It was my impression that many potential reviewers were put off by documentation and legal/structural issues, and stopped with their review at that point. Once you have move thes obstacles out of the way, you will likely draw more reviewers, and who knows what they have to say about the ring buffer. Regards, &rzej;
On 31/05/2017 09:38, Andrzej Krzemienski wrote:
3. Under the assumption that error_code_extended's use of a static global ring buffer would be controversial in this review, I hacked a quick and dirty solution expecting to have to remove it. Now we know that few disapprove, a more serious implementation will be needed. https://github.com/ned14/boost.outcome/issues/52
I am not sure of that. It was my impression that many potential reviewers were put off by documentation and legal/structural issues, and stopped with their review at that point. Once you have move thes obstacles out of the way, you will likely draw more reviewers, and who knows what they have to say about the ring buffer.
FWIW, I don't have a problem with the ring buffer per se, but I think it's currently too small to be effective.
On 30/05/2017 22:38, Andrzej Krzemienski via Boost wrote:
2017-05-30 16:58 GMT+02:00 Niall Douglas via Boost <boost@lists.boost.org>:
With regard to the previous high level summary at http://boost.2283326.n4.nabble.com/outcome-High-level- summary-of-review-feedback-accepted-so-far-tp4694767.html, I have made only these changes to the changes in that summary:
Thanks Niall, for the summary. But now, after these changes, the review I am still writing is useless, as it applies to a different library. Maybe we should schedule another review?
As I have said in a number of posts previously, I haven't been convinced to change much yet apart from drop some member functions for the runtime-checked empty-capable editions which are the ones presented for review. The behaviours of those are still being actively discussed e.g. should exception() synthesize an exception_ptr from an errored state? And even in the added varieties, it's not like I am writing any new source code. The statically checked varieties just leave out the runtime checks, otherwise they're literally identical consisting of identical implementation. The non-empty varieties just leave out the empty state. It's the same library as presented.
1. I have been persuaded to use longer more appropriate naming for result<T> and outcome<T>, so now the typedefed varieties with implicit conversions to their empty-capable form indicated by "=>" are:
- static_checked_outcome<T> => static_checked_optional_outcome<T> static_checked_result<T> => static_checked_optional_result<T>
No, no. I strongly suggest to provide only one outcome, with empty state if needed, and both narrow and wide contracts. We have learned to live with such types in C++, even if they are far from perfect.
I am absolutely sold on the empty-capable vs non-empty-capable being represented in the type system. Makes a big difference to public API contracts. I may choose to drop the typedef for the statically checked varieties, but the underlying non-public template is capable of all. I'll see what Charley's report recommends before deciding.
3. Under the assumption that error_code_extended's use of a static global ring buffer would be controversial in this review, I hacked a quick and dirty solution expecting to have to remove it. Now we know that few disapprove, a more serious implementation will be needed. https://github.com/ned14/boost.outcome/issues/52
I am not sure of that. It was my impression that many potential reviewers were put off by documentation and legal/structural issues, and stopped with their review at that point. Once you have move thes obstacles out of the way, you will likely draw more reviewers, and who knows what they have to say about the ring buffer.
Dunno, I got quite a few detailed questions on it, and people seemed pleased with the design rationale once they'd thought about it. Anyway, I intend to do something better than the current hack. Certainly with more storage, anyway. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
participants (10)
-
Andrzej Krzemienski -
Bjorn Reese -
Emil Dotchevski -
Gavin Lambert -
Gottlob Frege -
Niall Douglas -
Peter Dimov -
Robert Ramey -
Vicente J. Botet Escriba -
Vinnie Falco