[Boost][Outcome] Compile error?
When exceptions are disabled, I get: In file included from ../../../../boost/outcome/result.hpp:34: In file included from ../../../../boost/outcome/boost_result.hpp:36: In file included from ../../../../boost/exception_ptr.hpp:9: In file included from ../../../../boost/exception/detail/exception_ptr.hpp:17: ../../../../boost/exception/detail/clone_current_exception.hpp:22:6: error: This header requires exception handling to be enabled. Is there a way to configure Outcome so it does not use (boost) exception_ptr?
Emil Dotchevski wrote:
When exceptions are disabled, I get:
In file included from ../../../../boost/outcome/result.hpp:34: In file included from ../../../../boost/outcome/boost_result.hpp:36: In file included from ../../../../boost/exception_ptr.hpp:9: In file included from ../../../../boost/exception/detail/exception_ptr.hpp:17: ../../../../boost/exception/detail/clone_current_exception.hpp:22:6: error: This header requires exception handling to be enabled.
Are you reporting a bug to yourself?
On 04/07/2020 23:57, Peter Dimov via Boost wrote:
Emil Dotchevski wrote:
When exceptions are disabled, I get:
In file included from ../../../../boost/outcome/result.hpp:34: In file included from ../../../../boost/outcome/boost_result.hpp:36: In file included from ../../../../boost/exception_ptr.hpp:9: In file included from ../../../../boost/exception/detail/exception_ptr.hpp:17: ../../../../boost/exception/detail/clone_current_exception.hpp:22:6: error: This header requires exception handling to be enabled.
Are you reporting a bug to yourself?
Pretty much +1 from me as well. Surely we test Boost with exceptions globally disabled in the regression testing right? Niall
On Sat, Jul 4, 2020 at 5:58 PM Niall Douglas via Boost < boost@lists.boost.org> wrote:
On 04/07/2020 23:57, Peter Dimov via Boost wrote:
Emil Dotchevski wrote:
When exceptions are disabled, I get:
In file included from ../../../../boost/outcome/result.hpp:34: In file included from ../../../../boost/outcome/boost_result.hpp:36: In file included from ../../../../boost/exception_ptr.hpp:9: In file included from ../../../../boost/exception/detail/exception_ptr.hpp:17: ../../../../boost/exception/detail/clone_current_exception.hpp:22:6: error: This header requires exception handling to be enabled.
Are you reporting a bug to yourself?
Pretty much +1 from me as well.
Surely we test Boost with exceptions globally disabled in the regression testing right?
What Peter means is that boost::exception_ptr ought to support BOOST_NO_EXCEPTIONS. However, it does not, it never has. It is not trivial to support it because its implementation has been complicated over the years; remember, it was added when there was no standard support for exception_ptr. And I've learned that I can't remove old features because there's code in the wild that uses them. I think this is the complete list of legacy features: - If there is standard support, it is able to wrap std::exception_ptr in a boost::exception_ptr. - Otherwise, If the exception is a boost::exception, works correctly. - Otherwise, under old MSVC versions, it is able to talk to the ABI and clone the exception object correctly anyway. - Otherwise, it attempts a list of exception types as a fallback. Not sure what's the best approach here.
Emil Dotchevski wrote:
- If there is standard support, it is able to wrap std::exception_ptr in a boost::exception_ptr. - Otherwise, If the exception is a boost::exception, works correctly. - Otherwise, under old MSVC versions, it is able to talk to the ABI and clone the exception object correctly anyway. - Otherwise, it attempts a list of exception types as a fallback.
boost::current_exception() just returns {} when exceptions are disabled, so all this disappears. Only boost::copy_exception needs to be reimplemented to not rely on throw+current_exception. I'm not sure why it needs to do so even when exceptions are enabled though; the complete type is available at this point. Incidentally, the C++11 standard name for copy_exception is make_exception_ptr, but we never fixed it.
On 05/07/2020 03:29, Emil Dotchevski via Boost wrote:
Surely we test Boost with exceptions globally disabled in the regression testing right?
What Peter means is that boost::exception_ptr ought to support BOOST_NO_EXCEPTIONS. However, it does not, it never has.
I'll admit to surprise here. I always thought that one of the principle reasons for Boost.Exception is exactly so that Boost can work well enough with exceptions globally disabled. I mean, I know I've seen bits of Boost in use on embedded platforms where exceptions are globally disabled. It worked well enough. So you surprise me that I was wrong here.
It is not trivial to support it because its implementation has been complicated over the years; remember, it was added when there was no standard support for exception_ptr. And I've learned that I can't remove old features because there's code in the wild that uses them. I think this is the complete list of legacy features:
- If there is standard support, it is able to wrap std::exception_ptr in a boost::exception_ptr.
So now I am confused. If std::exception_ptr works well enough with exceptions globally disabled on all the major standard libraries, and Boost.Exception is able to wrap that, what's the problem here? Obviously, this is C++ 11 onwards only which Outcome requires in any case, but I'm not seeing what the showstopper is here. Coming back to your original question:
Is there a way to configure Outcome so it does not use (boost) exception_ptr?
The outcome/boost_result.hpp is just a specialisation of
outcome::basic_result<> with Boost types, same as outcome/std_result.hpp
or outcome/experimental/status_result.hpp is.
You can absolutely configure your own outcome::basic_result<>
specialisation. If you simply want to remove the boost::exception_ptr
dependency, I'd suggest cloning outcome/boost_result.hpp into a new
file, and quite literally comment out the Boost.Exception stuff.
Obviously by doing this you'd lose support for result
On Sun, Jul 5, 2020 at 4:35 AM Niall Douglas via Boost < boost@lists.boost.org> wrote:
On 05/07/2020 03:29, Emil Dotchevski via Boost wrote:
Surely we test Boost with exceptions globally disabled in the
regression
testing right?
What Peter means is that boost::exception_ptr ought to support BOOST_NO_EXCEPTIONS. However, it does not, it never has.
I'll admit to surprise here. I always thought that one of the principle reasons for Boost.Exception is exactly so that Boost can work well enough with exceptions globally disabled.
"Well enough" generally means that libraries are supposed (not even required) to always call boost::throw_exception to throw, which can be user-defined to do-something-and-not-return under BOOST_NO_EXCEPTIONS. On exception_ptr, my original thinking (more than 15 years ago) was this: the parts of Boost Exception that could be used to call boost::throw_exception should work under BOOST_NO_EXCEPTIONS, but since exceptions can't be thrown, there's no reason why anyone would want to hold a pointer to one. Outcome is the first library to hit that restriction.
You're the first person to ever raise a need for a outcome/boost_result.hpp which does not include Boost.Exception.
From my point of view, I was compiling an example that used outcome::result and was surprised that it didn't work under BOOST_NO_EXCEPTIONS. Even if we change boost::exception_ptr to work under BOOST_NO_EXCEPTONS, should outcome::result be coupled with it?
On 05/07/2020 19:41, Emil Dotchevski via Boost wrote:
On exception_ptr, my original thinking (more than 15 years ago) was this: the parts of Boost Exception that could be used to call boost::throw_exception should work under BOOST_NO_EXCEPTIONS, but since exceptions can't be thrown, there's no reason why anyone would want to hold a pointer to one.
What about make_exception_ptr()? There is a long story behind that function and whether it ought to work with exceptions globally disabled or not. Basically, many years ago, I reported to those STL implementations which were still implementing it as a throw...catch, instead of direct construction, benchmarks showing the hideous performance of not using direct construction. The maintainers of said STL implementations did not think it important enough to fix, but to my best knowledge, they have all since fixed that. This reduces the cost of std::make_exception_ptr() from 30k CPU cycles to about 3k CPU cycles, big gain if you do a lot of those. I'd pitch the same thing to you: thanks to make_exception_ptr(), exception_ptr is quite useful with exceptions globally disabled.
You're the first person to ever raise a need for a outcome/boost_result.hpp which does not include Boost.Exception.
From my point of view, I was compiling an example that used outcome::result and was surprised that it didn't work under BOOST_NO_EXCEPTIONS. Even if we change boost::exception_ptr to work under BOOST_NO_EXCEPTONS, should outcome::result be coupled with it?
I propose two paths forward: 1. Boost.Exception gains an exception_ptr which works fine with BOOST_NO_EXCEPTIONS. 2. Boost.Outcome disables use of boost::exception_ptr when BOOST_NO_EXCEPTIONS is enabled. I'm easy with either choice. But I think the former "nicer". Also, for the record, about 70% of my users use result<> with error or enum codes, perhaps 20% use exception_ptr, the remaining 10% use a custom type. Those who use exception_ptr are quite vocal, however. BTW if you are thinking of Boost.Exception improvements, that report going around for why Boost libraries cannot be epoch-ised, it said the only blocker for Outcome was its dependency on Exception. So, if Exception could be epoch-ised, so could Outcome. Me personally I give that whole proposal a very low priority, but while we're talking etc etc Niall
On Mon, Jul 6, 2020 at 7:14 AM Niall Douglas via Boost < boost@lists.boost.org> wrote:
On 05/07/2020 19:41, Emil Dotchevski via Boost wrote:
On exception_ptr, my original thinking (more than 15 years ago) was
this:
the parts of Boost Exception that could be used to call boost::throw_exception should work under BOOST_NO_EXCEPTIONS, but since exceptions can't be thrown, there's no reason why anyone would want to hold a pointer to one.
What about make_exception_ptr()?
There is a long story behind that function and whether it ought to work with exceptions globally disabled or not. Basically, many years ago, I reported to those STL implementations which were still implementing it as a throw...catch
You see, rather than fixing STL they should have fixed the damn compiler to not go full retard when it sees a throw. There is no reason why a throw followed by a catch should be slow. Fix that and we wouldn't need herbceptions or whatever. But I digress.
I propose two paths forward:
1. Boost.Exception gains an exception_ptr which works fine with BOOST_NO_EXCEPTIONS.
2. Boost.Outcome disables use of boost::exception_ptr when BOOST_NO_EXCEPTIONS is enabled.
I'm easy with either choice. But I think the former "nicer".
Not just nicer, it's the correct thing to do.
On 06/07/2020 22:24, Emil Dotchevski via Boost wrote:
I propose two paths forward:
1. Boost.Exception gains an exception_ptr which works fine with BOOST_NO_EXCEPTIONS.
2. Boost.Outcome disables use of boost::exception_ptr when BOOST_NO_EXCEPTIONS is enabled.
I'm easy with either choice. But I think the former "nicer".
Not just nicer, it's the correct thing to do.
We've done a half way house for the 1.74 release: Emil has added boost::make_exception_ptr(). This has enabled Outcome to dispense with including exception_ptr, if on Boost 1.74 or later, if C++ exceptions are globally disabled. Thus, Outcome should now compile in Boost 1.74 if C++ exceptions are globally disabled. Niall
Niall Douglas wrote:
We've done a half way house for the 1.74 release: Emil has added boost::make_exception_ptr().
Has he now? This is a potentially breaking change which is not allowed at this point. For reference, here's the release schedule: 06/24 closed for breaking changes 07/01 closed for major changes
On 07/07/2020 15:37, Peter Dimov via Boost wrote:
Niall Douglas wrote:
We've done a half way house for the 1.74 release: Emil has added boost::make_exception_ptr().
Has he now?
This is a potentially breaking change which is not allowed at this point. For reference, here's the release schedule:
06/24 closed for breaking changes 07/01 closed for major changes
We discussed whether adding an alternative naming for copy_exception() could be called a major change. You can see the commit at https://github.com/boostorg/exception/commit/330008445c38aa793a07909d623addf.... We concluded that it was so minor as to be unimportant, and for Outcome the relevant commit https://github.com/ned14/outcome/commit/eff410f5943e5acc16ab005e128096b27021... I'm classing as a minor bug fix. So, in my opinion, we're all good. Niall
Niall Douglas wrote:
We discussed whether adding an alternative naming for copy_exception() could be called a major change.
It is both major (API change) and potentially breaking (code that already defines its own make_exception_ptr may break). "We discussed" carries no weight unless one of the participants in said discussion was a release manager.
On 07/07/2020 16:05, Peter Dimov via Boost wrote:
Niall Douglas wrote:
We discussed whether adding an alternative naming for copy_exception() could be called a major change.
It is both major (API change) and potentially breaking (code that already defines its own make_exception_ptr may break).
Do you mean injecting a make_exception_ptr() into the boost namespace?
"We discussed" carries no weight unless one of the participants in said discussion was a release manager.
Our discussion was in good faith. Nothing was meant. If you want it rolled back, that's also fine. Niall
Am 07.07.20 um 17:19 schrieb Niall Douglas via Boost:
On 07/07/2020 16:05, Peter Dimov via Boost wrote:
Niall Douglas wrote:
We discussed whether adding an alternative naming for copy_exception() could be called a major change. It is both major (API change) and potentially breaking (code that already defines its own make_exception_ptr may break).
The commit in question is https://github.com/ned14/outcome/commit/eff410f5943e5acc16ab005e128096b27021... From API change view it conditionally removes a function from the detail namespace with no effect for the upcoming release. In the case BOOST_NO_EXCEPTIONS is defined it replaces an include of "boost/exception_ptr.hpp" with a forward declaration of that class. From what I understood "boost/exception_ptr.hpp" cannot be included when BOOST_NO_EXCEPTIONS is defined or compilation will fail. Hence I conclude that this is a pure bugfix (things compile that did not before) with no API or breaking change. As bugfix commits are still allowed according to the review schedule this is fine. Or am I missing anything?
On 07/07/2020 16:37, Alexander Grund via Boost wrote:
Am 07.07.20 um 17:19 schrieb Niall Douglas via Boost:
On 07/07/2020 16:05, Peter Dimov via Boost wrote:
Niall Douglas wrote:
We discussed whether adding an alternative naming for copy_exception() could be called a major change. It is both major (API change) and potentially breaking (code that already defines its own make_exception_ptr may break).
The commit in question is https://github.com/ned14/outcome/commit/eff410f5943e5acc16ab005e128096b27021...
From API change view it conditionally removes a function from the detail namespace with no effect for the upcoming release. In the case BOOST_NO_EXCEPTIONS is defined it replaces an include of "boost/exception_ptr.hpp" with a forward declaration of that class. From what I understood "boost/exception_ptr.hpp" cannot be included when BOOST_NO_EXCEPTIONS is defined or compilation will fail.
Hence I conclude that this is a pure bugfix (things compile that did not before) with no API or breaking change. As bugfix commits are still allowed according to the review schedule this is fine.
Or am I missing anything?
Yes, I think Peter's issue is with the commit to Boost.Exception, not to Boost.Outcome. Niall
On 07/07/2020 21:10, Emil Dotchevski via Boost wrote:
On Tue, Jul 7, 2020 at 9:11 AM Niall Douglas via Boost < boost@lists.boost.org> wrote:
Yes, I think Peter's issue is with the commit to Boost.Exception, not to Boost.Outcome.
Maybe that was a bad idea.
Well, I guess we'd better go ask permission from a release manager so, though I would still call that a "minor change". I had thought that by now one would have interceded with an opinion. Emil do you want to do the honours of emailing them? Also, we're just before Beta right? So, an ideal time to see if boost::make_exception_ptr() collides with user-injected boost::make_exception_ptr() would be for the beta release right? Though, to be honest, if there is collision, in my opinion that's a situation where users will just auto-fix their code silently and we'll never know. Users know not to be injecting new functions into not their namespaces. Niall
Niall Douglas wrote:
Also, we're just before Beta right? So, an ideal time to see if...
Why the rush? It's 9 years late, why not wait a few more months?
... boost::make_exception_ptr() collides with user-injected boost::make_exception_ptr() would be for the beta release right?
Adding things to boost:: is uncommon for user code, but an unqualified make_exception_ptr(x), where x has boost as an associated namespace, may become ambiguous or change behavior. Although defining boost::make_exception_ptr is not unheard of, either. See f.ex. https://github.com/boostorg/thread/blob/96cd717b331e62095aa58d6768dab3baa03f... which even does something different.
On 07/07/2020 23:03, Peter Dimov via Boost wrote:
Although defining boost::make_exception_ptr is not unheard of, either. See f.ex.
https://github.com/boostorg/thread/blob/96cd717b331e62095aa58d6768dab3baa03f...
which even does something different.
Well that there is rather more serious. Emil's commit should introduce a failure to compile Boost.Thread, by my reading of it. It's 11.15pm here and I must take my son for a haircut very early tomorrow morning, so I'm going to bed. Whatever you guys decide to do I'm happy with. Niall
On 2020-07-07 18:05, Peter Dimov via Boost wrote:
Niall Douglas wrote:
We discussed whether adding an alternative naming for copy_exception() could be called a major change.
It is both major (API change) and potentially breaking (code that already defines its own make_exception_ptr may break).
Not that I'm arguing with classification of this change as a major one, but I wouldn't consider it a breaking one as I don't think adding any symbols to a library namespace (except where explicitly allowed by contract) is a valid use of the library. This is the same stance as the standard C++ library is taking with regard to user's definitions in namespace std.
participants (5)
-
Alexander Grund -
Andrey Semashev -
Emil Dotchevski -
Niall Douglas -
Peter Dimov