What follows is my review of the Outcome library. I. DESIGN --------- The general idea of an error handling framework is sound, extremely appealing, and I found it well explained in the documentation. I like the concept very much and agree with many of the design decisions taken throughout. Specifically, constraining the error type to be the fixed std::error_code (or equivalent) and providing a wide accessor interface. I don't think that option<T> should have been included at all. It duplicates optional<T> and has no good reason to exist other than to showcase that the underlying policy-based implementation can produce it. I don't consider expected<T, E> a particularly needed part of the library; the focus should be on result<T> and outcome<T>, the two classes that represent the error handling philosophy on which the library is built. As already stated, I consider the interface of result<T> and outcome<T> a bit too cluttered. Not everyone shares my design preferences for minimal but complete interfaces, so there's legitimate argument to be had over whether, for instance, operator*, operator-> or value_or ought to be included, but the needless duplication between get() and value() is, well, needless. For an initial submission, it's better to err on the side of too little, rather than too much, because it's easy to add later, hard to remove. The empty state has been a source of controversy, and with reason. My initial stance was that it simply needs to go. This however presents a legitimate design question as to what the behavior of the default constructor ought to be. After seeing the suggestions of leaving the object uninitialized there, I now see the empty state as a significantly lesser evil. Having an empty state without the default constructor putting the object into it however is pointless. I do not agree with the direction things have taken of providing a multitude of result classes instead of a single one. Components that are intended to be used in interfaces should provide one, carefully designed option, which encourages what the designer deems best practices. The idea here, assuming that we shoot for standard acceptance at some point, is to provide std::result<T> and have APIs return it, not provide a bag of options and let everyone pick its own thing. (They already do that today and we're not better for it.) I do not agree with making value() unchecked by default. As I explained, one expression of the error handling philosophy behind the library is the following snippet: auto r = function().value(); where function() is noexcept and returns result<T>, whereas the place where the above line appears is a higher level component that signals errors not via error_code, but via exceptions. This one-liner provides the transition in an elegant way; losing it obscures the vision behind the library. Instead of using std::error_code, the library provides its own error_code_extended, complete with an interesting and very useful ring buffer logging mechanism that stores additional information along with the standard code/category values. Specifically, it returns error_code_extended from result<T>::error(). I would separate things a bit further here, attaching the additional information not to a separate error_code_extended class, but to result<> itself. That is, result<>::error() would return std::error_code, and I'd provide a separate accessor, result<>::error_info(), say, that would return the ring buffer entry associated with this result<>. This for me provides a cleaner separation between the basic result<> functionality, and the extended one, although this is a matter of taste. I would also support chaining result<>s by storing the index of the parent ring buffer entry in the current ring buffer entry. This is a straightforward and very useful extension. Instead of providing a macro BOOST_OUTCOME_CATCH_EXCEPTION_TO_RESULT, I would provide result<T>::set_error_from_exception(std::exception const& e); and outcome<T>::set_error_from_exception(std::exception const& e, std::exception_ptr const & p); that would do more or less what the macro does, except the fallback in the second case would be to store p instead of (EINVAL, e.what()). I'd also use make_error_result and make_value_result instead of make_errored_result and make_valued_result, as the former seem more grammatically correct. II. IMPLEMENTATION ------------------ The implementation, from a cursory look, appears of high quality, which is not surprising given that it was in use for two years. The preprocessed header is not easy to review, but the non-preprocessed form is available and amenable to study. Simple examples using outcome<T> compile and work well, although under VS2017 Clang/C2 windows.h is included, which is unfortunate but fixable. The library does not provide a test/Jamfile, which means that as submitted it will not integrate into Boost's testing infrastructure. Given that a Jamfile that performs the equivalent of withmsvc.bat is trivial to write: import testing ; project : requirements <include>../include/boost/outcome/v1.0 ; run unittests.cpp ; the omission is inexplicable. (Further integration into the Boost test matrix would ideally entail splitting unittests.cpp into separate .cpp tests so that success/failure can be tracked independently.) With the above Jamfile, trying to test the library with Cygwin gcc fails with: D:\tmp2\boost-master\libs\outcome\test>b2 toolset=gcc-cxx14 [...] gcc.compile.c++ ..\..\..\bin.v2\libs\outcome\test\unittests.test\gcc-cxx14-6.3.0\debug\unittests.o In file included from ../include/boost/outcome/outcome.hpp:26:0, from unittests.cpp:34: ../include/boost/outcome/outcome_v1.0.hpp:1446:22: fatal error: execinfo.h: No such file or directory #include <execinfo.h> ^ compilation terminated. I realize that Cygwin is not a supported platform, but this is what I have here and this is what I test with, so I figured I'll give it a try. Trying to test with VS 2017 fails with: D:\tmp2\boost-master\libs\outcome\test>b2 toolset=msvc-14.1 [...] compile-c-c++ ..\..\..\bin.v2\libs\outcome\test\unittests.test\msvc-14.1\debug\threading-multi\unittests.obj unittests.cpp unittests.cpp(780): error C3245: 'boost::outcome::_1_0_std_std_9274c0d4::basic_monad<boost::outcome::_1_0_std_std_9274c0d4::policy::monad_policy<udt>>::is_constructible': use of a variable template requires template argument list d:\tmp2\boost-master\libs\outcome\include\boost\outcome\outcome_v1.0.hpp(4271): note: see declaration of 'boost::outcome::_1_0_std_std_9274c0d4::basic_monad<boost::outcome::_1_0_std_std_9274c0d4::policy::monad_policy<udt>>::is_constructible' unittests.cpp(780): fatal error C1903: unable to recover from previous error(s); stopping compilation I get the same failure with /std:c++latest. Testing with the msvc-14.0 toolset works. The library as submitted contains both a preprocessed, fully self-contained, single header named outcome_v1_0.hpp, which is used by default, and a traditional non-preprocessed form that can be chosen with a macro. For the non-preprocessed form to be functional, a git submodule is incorporated by reference: [submodule "include/boost/outcome/boost-lite"] path = include/boost/outcome/boost-lite url = https://github.com/ned14/boost-lite.git branch = master fetchRecurseSubmodules = true ignore = untracked which in turn incorporates two more submodules: [submodule "include/gsl-lite"] path = include/gsl-lite url = https://github.com/martinmoene/gsl-lite.git branch = master fetchRecurseSubmodules = true ignore = untracked [submodule "pcpp"] path = pcpp url = https://github.com/ned14/pcpp.git branch = master fetchRecurseSubmodules = true ignore = untracked The first of these is not by the library author, and the second is a derived work. It is not possible for a Boost release, say, boost_1_65_0.zip, to incorporate git submodules by reference, so there remains a question of what, exactly, is being submitted for inclusion. My attempts to clarify this with the author have proven unsuccessful. I consider this a critical defect of the submission. It is simply not possible for a reviewer to give an opinion whether the submission ought to be included into Boost releases without it being known what, specifically, is being proposed to be included. There is no problem with the library containing implementation details under its directory, but we need to know what these details are. What the library actually needs as dependencies from boost-lite is not much and it's perfectly possible to extract the necessary parts and prepare a self-contained submission. This should have been done prior to the review. The files in the library contain the following: ``` Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License in the accompanying file Licence.txt or at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. Distributed under the Boost Software License, Version 1.0. (See accompanying file Licence.txt or copy at http://www.boost.org/LICENSE_1_0.txt) ``` This intends to dual license the library, but as written, it doesn't. What it does is apply the terms of BOTH licenses to distribution and use. This is also a critical defect. If the library is included in a Boost release, it should make it perfectly clear that it's licensed under the Boost license, and not under the intersection of Boost and Apache 2.0. There are several ways to do it, one is to clarify the above to specifically say "ASL2 or BSL, at your option" in the appropriate legalese, another is to license the Boost-included Outcome under BSL and the standalone Outcome under whatever. My recommendation would be to just license the lot under BSL and be done with it. Dual licensing creates unnecessary legal complications and may, from a legal point of view, be more burdensome than just using the BSL. Contributors need to be aware that they are licensing their patches under both licenses, it might not be very clear whether they have been aware, and so on. At times, the library duplicates certain functionality present elsewhere in Boost. I do not consider this a defect. It's a legitimate design decision to avoid intra-Boost dependencies, although the author needs to be aware of the downsides - using Boost components is basically taking advantage of free work instead of having to maintain everything oneself. III. DOCUMENTATION ------------------ I found the documentation perfectly adequate and was able to get a very good outline of how the classes were designed, how they were intended to be used, and what problem they intend to solve. It focuses a bit too much on expected<T, E>, which is not in itself a bad thing but is, for me, not exactly what the library is about. Fixing E to std::error_code is a fundamental (and correct) design decision from which these classes derive much of their utility. The Doxygen-generated reference will not win any awards, but it does the job. It however contains too many implementation details that would ideally either not be there at all, or tucked away in their own separate section. The monadic content is also a bit too high for my taste, and I would prefer if a user of result<> and outcome<> does not encounter the word monad at all. IV. VERDICT ----------- Should Outcome be accepted? In short, eventually. This, at present, is a NO. Ideally, what I would like to see is the library resubmitted in a conventional form, consisting of exactly the files that are intended to go into the libs/outcome directory in boost_1_65_0.zip, licensed under the BSL, only containing the files necessary for the non-preprocessed version of the code to compile and work. In addition, the test/ subdirectory would contain a Jamfile that is ready to integrate into the Boost test matrix.
The general idea of an error handling framework is sound, extremely appealing, and I found it well explained in the documentation. I like the concept very much and agree with many of the design decisions taken throughout. Specifically, constraining the error type to be the fixed std::error_code (or equivalent) and providing a wide accessor interface. [snip] I don't consider expected<T, E> a particularly needed part of the library; the focus should be on result<T> and outcome<T>, the two classes that represent the error handling philosophy on which the library is built.
FYI about half my potential user base want to set type E to their own type, and see setting E to error_code as highly retrograde because it loses type safety enforcement of disparate error code domains. It's why I implemented Expected. It doubles my potential user base easily.
I do not agree with the direction things have taken of providing a multitude of result classes instead of a single one. Components that are intended to be used in interfaces should provide one, carefully designed option, which encourages what the designer deems best practices.
As we have discussed, I prefer a competitive environment of alternatives. But if Vicente and I can come to agreement on naming of wide vs narrow observers, then we will only have: - result<T> and outcome<T> - without empty state - optional_result<T> and optional_outcome<T> - with empty state
The idea here, assuming that we shoot for standard acceptance at some point, is to provide std::result<T> and have APIs return it, not provide a bag of options and let everyone pick its own thing. (They already do that today and we're not better for it.)
I would recommend against standardising result<T> and outcome<T> in the strongest terms. Expected is the right design for the STL, not Outcome. I hope for SG14 to include Outcome in their standards quality collection of recommended libraries for low latency users, but in the non-standards-track category.
I would separate things a bit further here, attaching the additional information not to a separate error_code_extended class, but to result<> itself. That is, result<>::error() would return std::error_code, and I'd provide a separate accessor, result<>::error_info(), say, that would return the ring buffer entry associated with this result<>. This for me provides a cleaner separation between the basic result<> functionality, and the extended one, although this is a matter of taste.
I would also support chaining result<>s by storing the index of the parent ring buffer entry in the current ring buffer entry. This is a straightforward and very useful extension.
It is an interesting idea, but as I said at the time not as useful as you might think given the ephemeral nature of the storage. I am still surprised that nobody has yet objected strongly to storage which can vanish randomly during usage.
Instead of providing a macro BOOST_OUTCOME_CATCH_EXCEPTION_TO_RESULT, I would provide
result<T>::set_error_from_exception(std::exception const& e);
and
outcome<T>::set_error_from_exception(std::exception const& e, std::exception_ptr const & p);
that would do more or less what the macro does, except the fallback in the second case would be to store p instead of (EINVAL, e.what()).
That's a great idea for an API, and a variation on that has been logged to https://github.com/ned14/boost.outcome/issues/50. You may not have realised the main use case for BOOST_OUTCOME_CATCH_EXCEPTION_TO_RESULT: the elision under optimisation of C++ exception throws when using STL code. So for example: ``` result<void> function(std::vector<Foo> &a) noexcept { try { a.push_back(Foo()); } BOOST_OUTCOME_CATCH_EXCEPTION_TO_RESULT } ``` Here vector.push_back() might throw a std::bad_alloc. Under optimisation, the above code converts the throw of std::bad_alloc into a branch to early exit of an errored result<void>. So the exception throw and catch machinery is completely removed, and replaced with a fast branch. A very big win.
I'd also use make_error_result and make_value_result instead of make_errored_result and make_valued_result, as the former seem more grammatically correct.
I'll ponder that.
The library does not provide a test/Jamfile, which means that as submitted it will not integrate into Boost's testing infrastructure. Given that a Jamfile that performs the equivalent of withmsvc.bat is trivial to write:
import testing ;
project : requirements <include>../include/boost/outcome/v1.0 ;
run unittests.cpp ;
the omission is inexplicable.
The reason I omitted such a simple Jamfile is because it would be non-representative of the proper test suite which is CTest based. The withmsvc.bat file I placed there for reviewers to use in case they hate cmake so much they can't follow the simple ctest instructions at https://ned14.github.io/boost.outcome/introduction.html#unittests
(Further integration into the Boost test matrix would ideally entail splitting unittests.cpp into separate .cpp tests so that success/failure can be tracked independently.)
You may not be aware that all test results can be individually written to a JUnit XML file and thus fed to any test matrix. Indeed, the CTest Dashboard at http://my.cdash.org/index.php?project=Boost.Outcome already gives a test by test breakdown.
Trying to test with VS 2017 fails with:
As reported very early on in this review with a special announcement, Microsoft released VS2017.2 exactly at Outcome went to feature freeze. A specially fixed version of the review edition for VS2017.2 was placed at https://github.com/ned14/boost.outcome/tree/master_vs2017_fixed.
It is not possible for a Boost release, say, boost_1_65_0.zip, to incorporate git submodules by reference, so there remains a question of what, exactly, is being submitted for inclusion. My attempts to clarify this with the author have proven unsuccessful.
It would be trivially easy for the Boost release process to bring in git submodules recursively. There is no technical problem there, nor a legal one if submodules use compatible licences which is on me to get right (and I have). The sole obstacle is "that's not how things are done around here", with no technical justification. But I am surprised that you feel I did not clarify what is being submitted for inclusion. I have stated on multiple occasions that if submodules are a no-go, I would likely write a __has_include() to probe for the submodule, and if not present then I would fall back onto Boost. That way a single git repo can serve both as standalone and within Boost. Indeed I have written out several implementation scenarios for that situation, right down to the granular dependency file by dependency file level. I am not sure what more detail I can provide at this time.
I consider this a critical defect of the submission. It is simply not possible for a reviewer to give an opinion whether the submission ought to be included into Boost releases without it being known what, specifically, is being proposed to be included.
There is no problem with the library containing implementation details under its directory, but we need to know what these details are.
What the library actually needs as dependencies from boost-lite is not much and it's perfectly possible to extract the necessary parts and prepare a self-contained submission. This should have been done prior to the review.
It was not possible to know whether this library had any chance of acceptance prior to submission, nor what attitudes would be to using git submodules. I had genuinely expected people to question API design decisions, not get hung up on internal implementation libraries none of which interfere with any external nor Boost code in any way. They are a pure implementation detail, and in my opinion unimportant except regarding their provenance i.e. is their author a well known expert in C++ and highly likely to have written a high quality library? (the answer is yes by the way, unless you feel my libraries to be substandard. Martin's gsl-lite is great, but then he is Martin)
The files in the library contain the following:
``` Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License in the accompanying file Licence.txt or at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
Distributed under the Boost Software License, Version 1.0. (See accompanying file Licence.txt or copy at http://www.boost.org/LICENSE_1_0.txt) ```
This intends to dual license the library, but as written, it doesn't. What it does is apply the terms of BOTH licenses to distribution and use. This is also a critical defect. If the library is included in a Boost release, it should make it perfectly clear that it's licensed under the Boost license, and not under the intersection of Boost and Apache 2.0.
Would inserting this line between the stanzas suit you: ------------------------- OR --------------------------------- If so that's no problem, the licence boilerplate is all generated by script, it's done in a flash.
Should Outcome be accepted?
In short, eventually. This, at present, is a NO.
Ideally, what I would like to see is the library resubmitted in a conventional form, consisting of exactly the files that are intended to go into the libs/outcome directory in boost_1_65_0.zip, licensed under the BSL, only containing the files necessary for the non-preprocessed version of the code to compile and work. In addition, the test/ subdirectory would contain a Jamfile that is ready to integrate into the Boost test matrix.
This review seemed to me YES, but with the conditions you just listed. Can you explain why you chose NO? Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
``` result<void> function(std::vector<Foo> &a) noexcept { try { a.push_back(Foo()); } BOOST_OUTCOME_CATCH_EXCEPTION_TO_RESULT } ```
Here vector.push_back() might throw a std::bad_alloc. Under optimisation, the above code converts the throw of std::bad_alloc into a branch to early exit of an errored result<void>. So the exception throw and catch machinery is completely removed, and replaced with a fast branch. A very big win.
Unfortunately not. I expected this to be the case, but on the compilers I tested recently, the throw is not removed. Apparently the compilers consider the call to __cxa_throw observable behavior and never elide it. What compiler did? (I'll get back to the rest of your message later.)
Here vector.push_back() might throw a std::bad_alloc. Under optimisation, the above code converts the throw of std::bad_alloc into a branch to early exit of an errored result<void>. So the exception throw and catch machinery is completely removed, and replaced with a fast branch. A very big win.
Unfortunately not. I expected this to be the case, but on the compilers I tested recently, the throw is not removed. Apparently the compilers consider the call to __cxa_throw observable behavior and never elide it. What compiler did?
I wrote this little program https://godbolt.org/g/FIV8nB and tested it under GCC, clang and VS2017. I had thought that -flto or /GL (LTCG) folded blatantly obvious throw-try-catch stanzas like that because benchmarking of MSVC them showed them to be real fast. In hindsight, I should have also run a disassembler on the resulting binaries, what MSVC actually is doing is following an early out path in _CxxThrowException, I count maybe 20-30 instructions. _CxxThrowException does check for an installed handler before going down early out. One can of course install into SEH/TEH arbitrary interception handlers upon throw, this is exactly what the debugger does of course on all platforms. Hence I can see that the compiler vendors will consider the throw keyword to be always observable behaviour, just like an atomic access. That sucks. Thanks for catching this behaviour, I'll reform the documentation to correct this and add a FAQ entry. I'll rework the macro to call your suggested API instead, and may remove it altogether. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
I don't consider expected<T, E> a particularly needed part of the library; the focus should be on result<T> and outcome<T>, the two classes that represent the error handling philosophy on which the library is built.
FYI about half my potential user base want to set type E to their own type, and see setting E to error_code as highly retrograde because it loses type safety enforcement of disparate error code domains.
I don't object to expected<> being included; I just don't think that it has to have the spotlight. You're designing result<> and outcome<>, not expected<>, where you're tracking Vicente's papers. result/outcome represent a different paradigm.
I would recommend against standardising result<T> and outcome<T> in the strongest terms. Expected is the right design for the STL, not Outcome.
I don't agree here. One or both of result<T>/outcome<T> are a perfect fit for <system_error>. They expand the potential utility of std::error_code enormously and allow previously dual interfaces (f.ex. filesystem) to be expressed much more cleanly. Nothing against expected<>, I hope that we get it right as well. Horses for courses.
I would also support chaining result<>s by storing the index of the parent ring buffer entry in the current ring buffer entry. This is a straightforward and very useful extension.
It is an interesting idea, but as I said at the time not as useful as you might think given the ephemeral nature of the storage.
It's not as useless as you might think. Chaining results occurs during stack "unwinding" due to an error in a low level API. It returns a error result<>, the upper layer also returns an error, and so on upwards the stack, with some layers however deciding to replace the result<> with their own instead (as you do in your example.) At the uppermost level, you get an error result<> and it'd be extremely valuable if when logging the error you could follow the chain and log all intermediate results. For that, a ring buffer with 2048 entries (or however many you had) is plenty enough; there won't be 2048 failed operations in-between (well, there might be because threads, but you take what they give you; it can't get any worse than not having the information at all.)
I am still surprised that nobody has yet objected strongly to storage which can vanish randomly during usage.
The ring buffer hasn't gotten much review love. I like the idea. Although, as I mentioned, I'd have separated it slightly more from the rest so that it can be excised or reworked if necessary without touching the main API of result/outcome.
(Further integration into the Boost test matrix would ideally entail splitting unittests.cpp into separate .cpp tests so that success/failure can be tracked independently.)
You may not be aware that all test results can be individually written to a JUnit XML file and thus fed to any test matrix.
Yes, I looked at boost-lite's testing header and saw the JUnit XML support. But there are two reasons to split the test into separate files. One, if one of them fails to compile, you still get results in the matrix for the rest; and two, the Boost testing infrastructure doesn't do JUnit XML and is not expected (I think) to acquire such an ability in the near future. How it works is you list the tests in you Jamfile and the testing script then parses the b2 output and uploads the results. Here, for example: http://www.boost.org/development/tests/develop/developer/smart_ptr.html This is the Jamfile: https://github.com/boostorg/smart_ptr/blob/develop/test/Jamfile.v2 The test-suite and the [] can even be removed, run smart_ptr_test.cpp ; run weak_ptr_test.cpp ; and so on is all that's required.
It would be trivially easy for the Boost release process to bring in git submodules recursively. There is no technical problem there, ...
Boost releases do not contain foreign code. Everything in the release comes from boostorg repos, all of which are licensed under BSL. This is not just some sort of a whim. We've been building our reputation for years and Boost releases carry an implicit unwritten guarantee that we've vetted the code there both technically and legally. This was important for corporate adoption - there is a reason why when unspecified multinationals allow any outside code, it's most often Boost - and we carry a certain obligation to continue in the same manner or risk losing the trust we've gained. Incorporating foreign code from non-boostorg repos is a significant policy change. Steering committee + release managers significant. If you want to place your eggs in that basket, fine; I however don't think that a library review is the proper channel through which one effects such a policy change.
Indeed I have written out several implementation scenarios for that situation, right down to the granular dependency file by dependency file level. I am not sure what more detail I can provide at this time.
What you can provide is, quite simply, the proposed contents of the directory libs/outcome as it should appear in the next Boost release.
Would inserting this line between the stanzas suit you:
------------------------- OR ---------------------------------
Not really. There needs to be a prominent header before the two licenses that states unequivocally that the library is dual-licensed. Something like (but I'm not a lawyer) "Licensed under the Apache License, Version 2.0, or alternatively under the Boost Software License, Version 1.0. You may use the library according to the terms of either of those licenses, at your option." Even so, dual licensing is still an unnecessary obstacle you're erecting before your library. Just use the BSL and be done with it. I don't understand you love of setting precedents. This, too, is steering committee material, as everything else in Boost is BSL.
I don't consider expected<T, E> a particularly needed part of the > library; the focus should be on result<T> and outcome<T>, the two > classes that represent the error handling philosophy on which the > library is built.
FYI about half my potential user base want to set type E to their own type, and see setting E to error_code as highly retrograde because it loses type safety enforcement of disparate error code domains.
I don't object to expected<> being included; I just don't think that it has to have the spotlight. You're designing result<> and outcome<>, not expected<>, where you're tracking Vicente's papers. result/outcome represent a different paradigm.
Thing is, end users have clearly indicated that the majority isn't interested in my or your or Boost flavours of Expected. They want "the" Expected going into standards now. Much of that is a lack of awareness of how standards operate, they think that everything is like the Ranges TS in that you can be using Ranges right now if you go git submodule https://github.com/ericniebler/range-v3. They are looking for an equivalent for Expected in a git submodule. That's why the docs start from their perspective, and try to disabuse them of that starting point because it is possibly short sighted for their use case. I try to guide them from where a fair majority of them are starting from, into converting their custom error code enum into a proper error_category and using error_code instead. That doesn't suit all custom error code type use cases e.g. Emil's, but I completely agree with you that most of the time if they're not using error_code, they should be.
I would recommend against standardising result<T> and outcome<T> in the strongest terms. Expected is the right design for the STL, not Outcome.
I don't agree here. One or both of result<T>/outcome<T> are a perfect fit for <system_error>. They expand the potential utility of std::error_code enormously and allow previously dual interfaces (f.ex. filesystem) to be expressed much more cleanly.
Nothing against expected<>, I hope that we get it right as well. Horses for courses.
I'd support an addition to <system_error> like you suggest *after* Expected is into the standard and has had a bit of practice in the wild. We are in agreement that narrow observers are probably daft in an object mostly used for returning surprise. Vicente in the other thread appears to be unwilling to accept my request that good API design should always follow the principle of "less safety requires more programmer typing" which in my mind means operator*() needs to be wide, as does .value() and .error(). Let .unsafe_value() etc be the narrow editions. So I don't think I can reconcile Outcome with Expected now. But the reason for my caution regarding standardising std::result<T> is that maybe expected<T, E = std::error_code> with its narrow observers except for .value() will actually turn out to be fine in practice. You might then say "but if it is not, then the Filesystem TS and Networking TS with expected<T> return overloads will now have become unsafe" which is a valid worry. But then I'd not retrofit those with Expected yet. Too soon.
I would also support chaining result<>s by storing the index of the parent ring buffer entry in the current ring buffer entry. This is a straightforward and very useful extension.
It is an interesting idea, but as I said at the time not as useful as you might think given the ephemeral nature of the storage.
It's not as useless as you might think.
Chaining results occurs during stack "unwinding" due to an error in a low level API. It returns a error result<>, the upper layer also returns an error, and so on upwards the stack, with some layers however deciding to replace the result<> with their own instead (as you do in your example.)
At the uppermost level, you get an error result<> and it'd be extremely valuable if when logging the error you could follow the chain and log all intermediate results.
For that, a ring buffer with 2048 entries (or however many you had) is plenty enough; there won't be 2048 failed operations in-between (well, there might be because threads, but you take what they give you; it can't get any worse than not having the information at all.)
I think our disagreement solely stems from the ring buffer being just 16 entries long. If it were 2048 entries, then yes, I'd agree with you.
I am still surprised that nobody has yet objected strongly to storage which can vanish randomly during usage.
The ring buffer hasn't gotten much review love. I like the idea. Although, as I mentioned, I'd have separated it slightly more from the rest so that it can be excised or reworked if necessary without touching the main API of result/outcome.
As you may have noticed, I was misusing and abusing the lightweight logger in boost-lite to shoehorn in a quick and dirty implementation. I had been assuming that error_code_extended would be highly controversial, and I didn't want to waste time on a better implementation as I assumed I'd have to rip it out. Now I know it is not controversial, and it would drop the final hard dependency on boost-lite, it's definitely up for a proper local implementation. As was obvious in the list of files given to Bjorn, it's the only hard dependency for the header inclusion use case. The C++ exceptions disabled unit testing is the only other hard dependency remaining for the entire library.
Even so, dual licensing is still an unnecessary obstacle you're erecting before your library. Just use the BSL and be done with it. I don't understand you love of setting precedents. This, too, is steering committee material, as everything else in Boost is BSL.
Ah, but how boring would this review have been without a bit of thought provoking implementation decisions? After all, this is me. Firstly, thanks for the explanation above, it helped clarify things. I appreciate that your answer to my final question is going to be "just make it a normal Boost library", but let's say if you didn't answer that, I'd be interested on whether you'd prefer one of these two options: 1. Cronjob generated boostorg/outcome repo from ned14/outcome repo - Licence only BSL - No git submodules - No cmake, just bjam - No file clutter in root of repo as at present, or files unrelated to absolute minimum necessary for Boost 2. Same physical repo as standalone Outcome - Dual licensed - git submodules, but don't need to be checked out - cmake + bjam - Inevitable file clutter as so much tooling insists on files in specific locations Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
2017-05-29 16:38 GMT+02:00 Niall Douglas via Boost <boost@lists.boost.org>:
We are in agreement that narrow observers are probably daft in an object mostly used for returning surprise. Vicente in the other thread appears to be unwilling to accept my request that good API design should always follow the principle of "less safety requires more programmer typing" which in my mind means operator*() needs to be wide, as does .value() and .error(). Let .unsafe_value() etc be the narrow editions. So I don't think I can reconcile Outcome with Expected now.
I must protest. "less safety requires more programmer typing"-- I agree with this view. But artificially widening the contract doesn't make anything safer. If a programmer commits a bug, which is extracting the value without having verified that the value actually exists, it odes not make the program safe that you conceal this fact and instead apply *some* semantics to it: likely not the one that the programmer intended. If you have a narrow contract you leave a chance for static analyzers and UB-sanitizers to detect the bug. Widening contracts prevents such bug detection and is *unsafe*. You may still choose to go with wide contracts everywhere (and it might turn out to be the best choice), but accept that this is not a widely held view of safety. Don't call it "safety". Call it "wide contract". Regards, &rzej;
We are in agreement that narrow observers are probably daft in an object mostly used for returning surprise. Vicente in the other thread appears to be unwilling to accept my request that good API design should always follow the principle of "less safety requires more programmer typing" which in my mind means operator*() needs to be wide, as does .value() and .error(). Let .unsafe_value() etc be the narrow editions. So I don't think I can reconcile Outcome with Expected now.
I must protest. "less safety requires more programmer typing"-- I agree with this view. But artificially widening the contract doesn't make anything safer. If a programmer commits a bug, which is extracting the value without having verified that the value actually exists, it odes not make the program safe that you conceal this fact and instead apply *some* semantics to it: likely not the one that the programmer intended.
If you have a narrow contract you leave a chance for static analyzers and UB-sanitizers to detect the bug. Widening contracts prevents such bug detection and is *unsafe*. You may still choose to go with wide contracts everywhere (and it might turn out to be the best choice), but accept that this is not a widely held view of safety. Don't call it "safety". Call it "wide contract".
Darn. You now have me back on to thinking checked and unchecked typedefs are best. No v2 high level review of agreed changes after all. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
... I'd be interested on whether you'd prefer one of these two options:
1. Cronjob generated boostorg/outcome repo from ned14/outcome repo - Licence only BSL - No git submodules - No cmake, just bjam - No file clutter in root of repo as at present, or files unrelated to absolute minimum necessary for Boost
2. Same physical repo as standalone Outcome - Dual licensed - git submodules, but don't need to be checked out - cmake + bjam - Inevitable file clutter as so much tooling insists on files in specific locations
I think I favor option 1 from those two. It probably would be even nicer if you could make boostorg/outcome as described in (1) the primary and git submodule it into the standalone ned14/outcome, but (1) looks good enough.
I think I favor option 1 from those two. It probably would be even nicer if you could make boostorg/outcome as described in (1) the primary and git submodule it into the standalone ned14/outcome, but (1) looks good enough.
Thanks for that feedback. Not to chance my arm too much, but would you be willing to change your vote to yes conditional on form 1 and a second review to check that it is so? Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
I think I favor option 1 from those two. It probably would be even nicer if you could make boostorg/outcome as described in (1) the primary and git submodule it into the standalone ned14/outcome, but (1) looks good enough.
Thanks for that feedback.
Not to chance my arm too much, but would you be willing to change your vote to yes conditional on form 1 and a second review to check that it is so?
I suppose so, but as the design seems to be evolving as well, a proper second review will be in order and I can't promise you a YES vote there (which is what YES conditional basically means).
Not to chance my arm too much, but would you be willing to change your vote to yes conditional on form 1 and a second review to check that it is so?
I suppose so, but as the design seems to be evolving as well, a proper second review will be in order and I can't promise you a YES vote there (which is what YES conditional basically means).
The design hasn't changed much in truth. I just summarised it there to Vicente a few posts ago. But I accept you might want to see a written in stone design before accepting. Fair enough. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Le 29/05/2017 à 01:25, Niall Douglas via Boost a écrit :
The general idea of an error handling framework is sound, extremely appealing, and I found it well explained in the documentation. I like the concept very much and agree with many of the design decisions taken throughout. Specifically, constraining the error type to be the fixed std::error_code (or equivalent) and providing a wide accessor interface. [snip] I don't consider expected<T, E> a particularly needed part of the library; the focus should be on result<T> and outcome<T>, the two classes that represent the error handling philosophy on which the library is built.
Nial you remove always the part of the mail that say who sent it. We are unable to see it other than using some specific tool. Please, could you preserve the headers with the names in the discussion? Thanks in advance, Vicente
Nial you remove always the part of the mail that say who sent it. We are unable to see it other than using some specific tool.
I trim excessive quoted material as per Boost mailing list guidelines.
Please, could you preserve the headers with the names in the discussion?
Doesn't your mail client track this for you? If not, http://boost.2283326.n4.nabble.com/review-Review-of-Outcome-starts-Fri-19-Ma... shows a reasonable tree of threads. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Le 29/05/2017 à 17:47, Niall Douglas via Boost a écrit :
Nial you remove always the part of the mail that say who sent it. We are unable to see it other than using some specific tool. I trim excessive quoted material as per Boost mailing list guidelines. You believe that preserving the names of the people is excessive? It has no special cost for you, just preserve 1 or two lines. I wonder who is excessive here. How many posts do you see in this ML that do the way you do?
Please, could you preserve the headers with the names in the discussion? Doesn't your mail client track this for you?
If not, http://boost.2283326.n4.nabble.com/review-Review-of-Outcome-starts-Fri-19-Ma... shows a reasonable tree of threads.
This is a question of respect for the people that have written the mail and the people that are reading them. It has no special cost for you, just preserve 1 or two lines. I'm using Thunderbird. Sometimes, I don't know why, some posts are not attached to the thread it was a response of. This was the case of your mail. I don't know why I would need to look at some external pages in order to see to what this post is a response of. What costs to you to left these 2 lines in the post? Vicente
participants (4)
-
Andrzej Krzemienski -
Niall Douglas -
Peter Dimov -
Vicente J. Botet Escriba