Reforming Boost.System and <system_error> round 2
Many thanks to those who have contributed to the discussion in the other thread regarding making a breaking change to Boost.System in order to provide empirical test data to persuade WG21 to similarly break <system_error> in a future C++ standard for its own good. This fresh email is hopefully to start finishing the discussion. To summarise discussion to date, Peter has made Boost.System's error_code and error_category constexpr except for the virtual functions. A big win for codegen, and the choice of the default constructor of error_code to use system_category now no longer prevents constexpr, so the request for a new special null_category() can also be dropped. There is substantial opposition to making message() return string_view, so let's drop that request. That leaves the request to fix "if(ec) ..." which right now returns true if the value is 0, despite that much code writes "if(ec) ..." to mean "if error then ...". There is also the issue of error coding schemes not being able to have more than one success value, which usually must be 0. That's the remaining discussion point. Myself and Peter feel this is worth fixing despite the increased runtime overhead. Others, including one of its original inventors, feel it is too much overhead. To add some empirical evidence to the discussion, I have coded up a mock up of the improved error_code which can be viewed at https://godbolt.org/g/dDAWih. This demonstrates constexpr error_code and error_category in action as well. To summarise what the mock up demonstrates: 1. error_category() gains a new virtual function called `bool failure(int code)` which is now called by error_code's operator bool. 2. Thanks to the new constexpr construction, the compiler now completely elides from runtime the case where an inlined function returns an error code and the caller acts upon it. As in, **zero code is now generated**. Far better than before. No virtual functions are called, they are optimised out entirely. 3. For unknown error codes from extern sources where the linker is generating an executable, the "final" modifier removes a layer of indirection when calling `failure()` after the linker has run, and indeed clang seems to go ahead and inline the implementation entirely despite us not asking for LTO. Which is kinda cool. 4. Thus only in shared objects, and only for unknown error codes from extern sources, is a virtual function call actually performed. And given the frequency of doing `if(ec) ...`, it'll almost certainly be hot in the branch predictor i.e. no stalls. Question: is this still considered too much overhead? Note that there is no longer ANY proposed breaking change with existing code. This is now 100% enhancement only. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 1/15/18 1:18 PM, Niall Douglas via Boost wrote:
That leaves the request to fix "if(ec) ..." which right now returns true if the value is 0, despite that much code writes "if(ec) ..." to mean "if error then ...". There is also the issue of error coding schemes not being able to have more than one success value, which usually must be 0. That's the remaining discussion point.
I apologize for parachuting into the discussion ... But ... Can't one just have "if(ec) ..." invoke a syntax error? Perhaps by deleting or doing a static_assert on the conversion to bool? This would require that the user use "if(ec == my_error_code::success) or if(ec == my_error_condition::stupid_error) or whatever. This would eliminate lot's of stupid errors and leave the code simpler. It would also eliminate hours and hours and hours of discussion as too what "if(ec) ... " should mean. Robert Ramey
On 01/16/18 02:37, Robert Ramey via Boost wrote:
On 1/15/18 1:18 PM, Niall Douglas via Boost wrote:
That leaves the request to fix "if(ec) ..." which right now returns true if the value is 0, despite that much code writes "if(ec) ..." to mean "if error then ...". There is also the issue of error coding schemes not being able to have more than one success value, which usually must be 0. That's the remaining discussion point.
I apologize for parachuting into the discussion ... But ...
Can't one just have "if(ec) ..." invoke a syntax error?
Obviously, this will break lots of code for no good reason. I think, the understanding of the meaning of such tests is more or less established. The problem is to make it support additional use cases without penalizing the existing code too much.
Rather than change the meaning of the (ambiguous but no doubt ubiquitous) bool conversion, might it not be better to offer a new method? if (ec.is_error()) { // ... } is_error() could consult the error_category. or better, ADL: if (is_error(ec)) { // ... } re performance arguments: The only time the performance cost a virtual call is relevant is when the function is called in a tight loop. error_code is the result of a system or system-like call. In a reasonable program, it's never called in a tight loop. On 16 January 2018 at 14:30, Andrey Semashev via Boost < boost@lists.boost.org> wrote:
On 01/16/18 02:37, Robert Ramey via Boost wrote:
On 1/15/18 1:18 PM, Niall Douglas via Boost wrote:
That leaves the request to fix "if(ec) ..." which right now returns true
if the value is 0, despite that much code writes "if(ec) ..." to mean "if error then ...". There is also the issue of error coding schemes not being able to have more than one success value, which usually must be 0. That's the remaining discussion point.
I apologize for parachuting into the discussion ... But ...
Can't one just have "if(ec) ..." invoke a syntax error?
Obviously, this will break lots of code for no good reason. I think, the understanding of the meaning of such tests is more or less established. The problem is to make it support additional use cases without penalizing the existing code too much.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman /listinfo.cgi/boost
Richard Hodges wrote:
Rather than change the meaning of the (ambiguous but no doubt ubiquitous) bool conversion, might it not be better to offer a new method?
if (ec.is_error()) ...
Or ec.failed(). I doubt that we'll be able to change the meaning of `if(ec)` in the standard. Although who knows, it's a compatible change in the case nobody overrides error_category::failed. Which they don't because it doesn't exist yet. I don't much like `if(ec)`, it's a bit counter-intuitive that it means "failed" and not "succeeded". But it's short and idiomatic.
On 01/16/18 17:06, Peter Dimov via Boost wrote:
Richard Hodges wrote:
Rather than change the meaning of the (ambiguous but no doubt ubiquitous) bool conversion, might it not be better to offer a new method?
if (ec.is_error()) ...
Or ec.failed().
I doubt that we'll be able to change the meaning of `if(ec)` in the standard. Although who knows, it's a compatible change in the case nobody overrides error_category::failed. Which they don't because it doesn't exist yet.
I don't much like `if(ec)`, it's a bit counter-intuitive that it means "failed" and not "succeeded". But it's short and idiomatic.
If you spell it "if (err)" or "if (error)", it is very self-explanatory, IMHO. "if (error.failed())" makes it more verbose without adding any clarity.
On 16/01/2018 13:55, Richard Hodges via Boost wrote:
Rather than change the meaning of the (ambiguous but no doubt ubiquitous) bool conversion, might it not be better to offer a new method?
if (ec.is_error()) { // ... }
Why can't those who really, really want to avoid any potential virtual function call in `if(ec) ...` simply write: if(ec.value() != 0) ... No new member functions needed. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 1/16/18 5:30 AM, Andrey Semashev via Boost wrote:
On 01/16/18 02:37, Robert Ramey via Boost wrote:
On Can't one just have "if(ec) ..." invoke a syntax error?
Obviously, this will break lots of code for no good reason.
Truth is - If I see this I don't know what it actually means. I bet that half the code that would fail to compile is wrong. So I would argue that this would UNbreak lots of code.
I think, the understanding of the meaning of such tests is more or less established.
not to me. But maybe that's just me. Robert Ramey
2018-01-16 15:22 GMT+01:00 Robert Ramey via Boost <boost@lists.boost.org>:
On 1/16/18 5:30 AM, Andrey Semashev via Boost wrote:
On 01/16/18 02:37, Robert Ramey via Boost wrote:
On Can't one just have "if(ec) ..." invoke a syntax error?
Obviously, this will break lots of code for no good reason.
Truth is - If I see this I don't know what it actually means. I bet that half the code that would fail to compile is wrong. So I would argue that this would UNbreak lots of code.
I think, the understanding of the meaning of such tests is more or less
established.
not to me. But maybe that's just me.
The semantics of this `if(ec)` is "conditionally well-defined": provided that you make sure that in all error categories you use throughout your program zero indicates success and non-zero values indicate failure, then (and only then) does this construct test if `ec` represents a failure. Some programs can guarantee that, e.g. if they only use the default category on Unix. Regards, &rzej;
The semantics of this `if(ec)` is "conditionally well-defined": provided that you make sure that in all error categories you use throughout your program zero indicates success and non-zero values indicate failure, then (and only then) does this construct test if `ec` represents a failure.
Forgive me for the noise in case I've missed something, but if the primary concern is that we want to be able to support non-zero success codes, why not just make the error_category carry the int representation of success instead of assuming that it's always zero? It's not as generic as introducing a new virtual that allows one to do whatever (for example there would still be one blessed value, so you couldn't have different 'success' values), but it's almost certainly faster than a virtual call, and might satisfy most of the use cases. -- chris
The semantics of this `if(ec)` is "conditionally well-defined": provided that you make sure that in all error categories you use throughout your program zero indicates success and non-zero values indicate failure, then (and only then) does this construct test if `ec` represents a failure.
Some programs can guarantee that, e.g. if they only use the default category on Unix.
However there are plenty of C error coding schemes with many values for success e.g. SUCCESS_QUOTA_LOW SUCCESS_SLOWPATH SUCCESS_VIA_BROKER ... and so on. C++ 11's error_code's `if(ec) ...` doesn't handle these, and as Chris mentioned, it was never designed to do so. Though error_code itself handles them perfectly fine. Originally, `if(ec)` meant "if error code". Of course, recent C++ isn't so sloppy anymore, we no longer should write `if(x)` when we mean `if(x != 0)`. And clang-tidy et al warns now appropriately. That cultural shift in best practice is part of the cause of this debate with `if(ec) ...`. The programmer is writing that, and usually expecting it means something that it currently does not. After all, testing a smart pointer by boolean check means "is the smart pointer pointing to something?" So are we reaching a consensus towards or away from the proposed change? Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
2018-01-16 15:59 GMT+01:00 Niall Douglas via Boost <boost@lists.boost.org>:
The semantics of this `if(ec)` is "conditionally well-defined": provided that you make sure that in all error categories you use throughout your program zero indicates success and non-zero values indicate failure, then (and only then) does this construct test if `ec` represents a failure.
Some programs can guarantee that, e.g. if they only use the default category on Unix.
However there are plenty of C error coding schemes with many values for success e.g.
SUCCESS_QUOTA_LOW SUCCESS_SLOWPATH SUCCESS_VIA_BROKER
... and so on. C++ 11's error_code's `if(ec) ...` doesn't handle these, and as Chris mentioned, it was never designed to do so. Though error_code itself handles them perfectly fine.
Originally, `if(ec)` meant "if error code". Of course, recent C++ isn't so sloppy anymore, we no longer should write `if(x)` when we mean `if(x != 0)`. And clang-tidy et al warns now appropriately.
That cultural shift in best practice is part of the cause of this debate with `if(ec) ...`. The programmer is writing that, and usually expecting it means something that it currently does not. After all, testing a smart pointer by boolean check means "is the smart pointer pointing to something?"
So are we reaching a consensus towards or away from the proposed change?
How about annotating the conditional conversion to bool deprecated? It still compiles and works as before, but with compiler flag `-Werror=deprecated` can be turned into compiler error on demand, and then you are forced to rewrite to either: ``` if (ec.value() != 0) ``` or: ``` if (ec.failure()) ``` ? Regards, &rzej;
On 01/16/18 18:35, Andrzej Krzemienski via Boost wrote:
How about annotating the conditional conversion to bool deprecated? It still compiles and works as before, but with compiler flag `-Werror=deprecated` can be turned into compiler error on demand, and then you are forced to rewrite to either:
``` if (ec.value() != 0) ```
This is not correct if we're going to accept Niall's proposal.
or:
``` if (ec.failure()) ```
?
I really don't see the point of this.
2018-01-16 16:39 GMT+01:00 Andrey Semashev via Boost <boost@lists.boost.org> :
On 01/16/18 18:35, Andrzej Krzemienski via Boost wrote:
How about annotating the conditional conversion to bool deprecated? It still compiles and works as before, but with compiler flag `-Werror=deprecated` can be turned into compiler error on demand, and then you are forced to rewrite to either:
``` if (ec.value() != 0) ```
This is not correct if we're going to accept Niall's proposal.
Correct. This is an alternative rather than complement to Niall's proposal.
or:
``` if (ec.failure()) ```
?
I really don't see the point of this.
This attempts to make everyone happy. You can still use the old syntax (conversion to bool) with old semantics. Others can request the compiler to look for potentially buggy usages of conversion operator. There is a way to check fast for zero-code (regardless if it means failure of success -- and you state your intentions explicitly) and we get function `failure()` that explicitly says you want to check for failure condition even at the expense of a virtual function call. Regards, &rzej;
On Tue, Jan 16, 2018 at 9:47 AM, Andrzej Krzemienski via Boost < boost@lists.boost.org> wrote:
This attempts to make everyone happy.
Then the attempt to *annotate the conditional conversion to bool deprecated* fails. While it makes the people who love verbosity happy, it does so at a cost of breaking perfectly correct, well-tested code, and I suspect a large amount of code at that. Heck, it even breaks Boost code. I would guess that filesystem, asio and dll are some of the biggest users of error_code, and not only do users of those libraries regularly use "if (ec)", those libraries themselves internally use the same construct. -- Nevin ":-)" Liber <mailto:nevin@cplusplusguy.com <nevin@eviloverlord.com>> +1-847-691-1404
On 1/16/18 8:56 AM, Nevin Liber via Boost wrote:
On Tue, Jan 16, 2018 at 9:47 AM, Andrzej Krzemienski via Boost < boost@lists.boost.org> wrote:
This attempts to make everyone happy.
Then the attempt to *annotate the conditional conversion to bool deprecated* fails.
While it makes the people who love verbosity happy, it does so at a cost of breaking perfectly correct, well-tested code, and I suspect a large amount of code at that.
a) True - it would break code by invoking a compile time error b) which would be trivial to fix c) and likely in some cases smoke out some errors Heck, it even breaks Boost code. I would guess that
filesystem, asio and dll are some of the biggest users of error_code, and not only do users of those libraries regularly use "if (ec)", those libraries themselves internally use the same construct.
d) So we would find some new bugs in Boost as well. Sounds like a feature to me. Robert Ramey
On Tue, Jan 16, 2018 at 12:44 PM, Robert Ramey via Boost < boost@lists.boost.org> wrote:
On 1/16/18 8:56 AM, Nevin Liber via Boost wrote:
On Tue, Jan 16, 2018 at 9:47 AM, Andrzej Krzemienski via Boost < boost@lists.boost.org> wrote:
This attempts to make everyone happy.
Then the attempt to *annotate the conditional conversion to bool deprecated* fails.
While it makes the people who love verbosity happy, it does so at a cost of breaking perfectly correct, well-tested code, and I suspect a large amount of code at that.
a) True - it would break code by invoking a compile time error b) which would be trivial to fix c) and likely in some cases smoke out some errors
Heck, it even breaks Boost code. I would guess that
filesystem, asio and dll are some of the biggest users of error_code, and not only do users of those libraries regularly use "if (ec)", those libraries themselves internally use the same construct.
d) So we would find some new bugs in Boost as well. Sounds like a feature to me.
Robert Ramey
If you decide to change system_error / error_code please make it header-only. This is the only part of Boost.System that requires a library, and I had to work around that fact in Boost.Uuid by providing my own exception type when entropy generation errors occur, where I would have much preferred to leverage system_error, but since Uuid is a header-only solution, I could not. - Jim
James E. King, III wrote:
If you decide to change system_error / error_code please make it header-only.
We went over this last time, and touched the question again this time; the specification of system_category() requires that a reference to the same object is returned, and in the presence of DLLs, the only way to guarantee this is to make Boost.System itself a DLL. If we adopt a more relaxed attitude and change operator== for error_category to match (by using a similar technique there to what MS does in their <system_error>), we might be able to get header-only working in practice in this scenario (if one doesn't pay attention to the actual address of the category.) But even if we do fix the header-only case, switching to header-only by default would still be a major change.
On Tue, Jan 16, 2018 at 11:44 AM, Robert Ramey via Boost < boost@lists.boost.org> wrote:
On 1/16/18 8:56 AM, Nevin Liber via Boost wrote:
On Tue, Jan 16, 2018 at 9:47 AM, Andrzej Krzemienski via Boost < boost@lists.boost.org> wrote:
This attempts to make everyone happy.
Then the attempt to *annotate the conditional conversion to bool deprecated* fails.
While it makes the people who love verbosity happy, it does so at a cost of breaking perfectly correct, well-tested code, and I suspect a large amount of code at that.
a) True - it would break code by invoking a compile time error b) which would be trivial to fix
For people who don't have to pay. Churn for the sake of churn is costly for those who have to pay for testing, external software verification, etc.
c) and likely in some cases smoke out some errors
Please provide some evidence to back up this assertion. If the premise is that filesystem, asio and dll and the dominant use cases, I'm not seeing any errors. And if that isn't the premise, then what are the dominant use cases for error_code?
Heck, it even breaks Boost code. I would guess that
filesystem, asio and dll are some of the biggest users of error_code, and not only do users of those libraries regularly use "if (ec)", those libraries themselves internally use the same construct.
d) So we would find some new bugs in Boost as well. Sounds like a feature to me.
My spot-checking of this idiom in Boost finds no bugs. Please provide evidence of a bug in Boost caused by this. Heck, Boost example code advertises this is the correct way to use error_code. -- Nevin ":-)" Liber <mailto:nevin@eviloverlord.com> +1-847-691-1404
On 1/16/18 10:06 AM, Nevin Liber via Boost wrote:
For people who don't have to pay. Churn for the sake of churn is costly for those who have to pay for testing, external software verification, etc.
I don't think it's just for the sake of churn. I think it's to improve program correctness by replacing ambiguous and opaque implicit behavior with unambiguous explicit behavior which is obviously correct on it's face without having to go look it in some documentation or source code somewhere else.
c) and likely in some cases smoke out some errors
Please provide some evidence to back up this assertion.
LOL - I can't "prove" that its going to smoke out some error. But that's not the point. One can't "prove" that the current usage is correct by looking at it. My experience is that something which is not obviously correct eventually comes to be seen as incorrect.
If the premise is that filesystem, asio and dll and the dominant use cases,
This is not a premise for me.
I'm not seeing any errors.
Of course you're not. The current API hides any which might be there. That's exactly my complaint. You cannot prove there are no errors in the usage simply by observation.
And if that isn't the premise, then what are the dominant use cases for error_code?
I see error_code as a way to reconcile varying lists of error codes in a systematic way which will avoid confusion which often leads to subtle, hard to find, bugs. Certainly I see it as much more general then three boost libraries.
Heck, it even breaks Boost code. I would guess that
filesystem, asio and dll are some of the biggest users of error_code, and not only do users of those libraries regularly use "if (ec)", those libraries themselves internally use the same construct.
The fact that something is in wide usage is not enough to convince me that it's a good idea.
d) So we would find some new bugs in Boost as well. Sounds like a feature to me.
My spot-checking of this idiom in Boost finds no bugs. Please provide evidence of a bug in Boost caused by this.
see above
Heck, Boost example code advertises this is the correct way to use error_code.
Maybe so, but that doesn't convince me that it's a good idea. Robert Ramey
On 01/16/18 21:22, Robert Ramey via Boost wrote:
I don't think it's just for the sake of churn. I think it's to improve program correctness by replacing ambiguous and opaque implicit behavior with unambiguous explicit behavior which is obviously correct on it's face without having to go look it in some documentation or source code somewhere else.
There's nothing ambuguous about the conversion operator, as it is specified in the standard, and I find the syntax quite intuitive. You're not improving correctness by replacing it with a regular method. Instead, you're adding verbosity. Whether that is a good thing or not is a matter of taste. Personally, I choose the shorter syntax in this case. Also, simply replacing one construct with another doesn't "fix" the code. On the opposite, it has the potential of breaking it.
2018-01-16 19:58 GMT+01:00 Andrey Semashev via Boost <boost@lists.boost.org> :
On 01/16/18 21:22, Robert Ramey via Boost wrote:
I don't think it's just for the sake of churn. I think it's to improve program correctness by replacing ambiguous and opaque implicit behavior with unambiguous explicit behavior which is obviously correct on it's face without having to go look it in some documentation or source code somewhere else.
There's nothing ambuguous about the conversion operator, as it is specified in the standard, and I find the syntax quite intuitive.
I disagree here. The sole fact that we are pursuing this thread proves that contextual conversion to bool for error_code is ambiguous. You get professional programmers that were convinced or expect that this conversion tells failure from success. You have others that say they have designed error_code with the intention in mind that 0 has nothing to do with "success". The fact that its semantics are clearly described in the standard does not resolve ambiguity or error prone-ness.The semantics of the following code are also well defined: ``` int i = f(); // ... if (i = 0) // just one = i = g(); ``` Maybe I know what I am doing. But compilers nonetheless chose to issue a warning here, because it is _likely_ that the program does something else than intended. I am looking at the problem from the perspective of a programmer that works on the same code base with 20 other programmers. When I review the code I need to know what the intentions of my colleague was. I need it expressed so that I do not have concerns whether he is doing it deliberately or accidentally. You say you find the syntax quite intuitive. But do you also find the semantics intuitive? When you type or read `if(ec)` do you interpret it as "if operation failed" or as "if operation returned code of numeric value zero, regardless of the domain and regardless of whether in this domain zero means success or failure"?
You're not improving correctness by replacing it with a regular method. Instead, you're adding verbosity.
It is not about correctness, or rather not directly about correctness. It is about stating your intentions unambiguously in the code. So that it does not only compile to the correct code, but also clearly communicate your intentions.
Whether that is a good thing or not is a matter of taste. Personally, I choose the shorter syntax in this case.
True, in the end it is just a choice.
Also, simply replacing one construct with another doesn't "fix" the code.
True, but it is not about "fixing", it is about expressing your intent. Given that conversion to bool has semantics confusing to many programmers, and that there will be a reluctance to change it for the fear of breaking backwards compatibility or degraded performance, providing a different name seems like a reasonable option. On the opposite, it has the potential of breaking it. It is clear to me that class `error_code` means two quite incompatible things. One is "just status" -- success or failure, or something in between. The second one is "reason for failure" (failure implied). The former was intended in the initial design. But there are things that indicate the latter: name "error", or conversion to bool, which conventionally means "valid or not?", "interesting or not?", "populated or not?". Users do not get clear message whether they should treat it as "reason for failure" or "just status". Regards, &rzej;
On Wed, Jan 17, 2018 at 1:16 AM, Andrzej Krzemienski via Boost <boost@lists.boost.org> wrote:
2018-01-16 19:58 GMT+01:00 Andrey Semashev via Boost <boost@lists.boost.org> :
There's nothing ambuguous about the conversion operator, as it is specified in the standard, and I find the syntax quite intuitive.
You say you find the syntax quite intuitive. But do you also find the semantics intuitive? When you type or read `if(ec)` do you interpret it as "if operation failed" or as "if operation returned code of numeric value zero, regardless of the domain and regardless of whether in this domain zero means success or failure"?
The two interpretations are equivalent for me, because in my code error code of zero always means "success". Why? Because I pick the error values that way (or map them that way, if the values come from an external API) so that they play nicely with `error_code` design and the rest of the code. If we support multiple "success" values or non-zero success values, I would expect "if (err)" to mean "if some error happened". Why? Because that "if" controls error handling, not an arbitrary non-zero result handling. I've never seen or written code that does otherwise. If I want to test for a particular error code, I would write "if (err == x)" or "if (err != x)" - that's the syntax that communicates the intent to check for the particular value, including when `x` happens to be zero (which doesn't really matter, because `x` is always an enum value and never a magic number). You may ask the what is the difference between "if (err)" and "if (err != success)" and it is that the latter only tests for failure in the particular domain. Now, this difference may not be obvious, and that is why I generally avoid writing "if (err != success)". Maybe I'm missing some crucial experience, but so far the convention I described above worked well for me.
-----Original Message----- From: Boost [mailto:boost-bounces@lists.boost.org] On Behalf Of Andrey Semashev via Boost Sent: 16 January 2018 18:59 To: boost@lists.boost.org Cc: Andrey Semashev Subject: Re: [boost] Reforming Boost.System and <system_error> round 2
<snip>
"There's nothing ambuguous about the conversion operator, "
You seem to have serendipitously invented a new word ' ambuguous ' that aptly describes this whole sorry mess ;-) Coming from outside C, (especially VMS where success == 1, not 0) if(ec) was and is, for me, definitely ambuguous! and other C-savvy posters in this thread confirm. No solution is going to be painless. I'd avoid trying to force any retrofit (because of downstream revalidation costs), but enforce use of Outcome (and >= C++11) on new libraries. Paul --- Paul A. Bristow Prizet Farmhouse Kendal UK LA8 8AB +44 (0) 1539 561830
The semantics of this `if(ec)` is "conditionally well-defined": provided that you make sure that in all error categories you use throughout your program zero indicates success and non-zero values indicate failure, then (and only then) does this construct test if `ec` represents a failure.
Some programs can guarantee that, e.g. if they only use the default category on Unix. However there are plenty of C error coding schemes with many values for success e.g.
SUCCESS_QUOTA_LOW SUCCESS_SLOWPATH SUCCESS_VIA_BROKER I believe error_code shouldn't be used for this case. We need something else as a status_code type that conveys success/failure information. There is a standard proposal status_value that conveys a status and
Le 16/01/2018 à 15:59, Niall Douglas via Boost a écrit : optionally a value if the status represents a success. If we split a status on two types one for the success states and the other for the failure states, status_value<S,T> can be seen as expected<pair<Success<S>, T>, Failure<S>> but with a different representation. Note that when there is a single value for success, there is no need to represent it, as expected<T, Failure> IMHO, If we want to transport success states they must be transported on the success alternative, not on the failure one. Best, Vicente P.S. I know that this post is not about Outcome, but I believe the motivation comes from your Outcome design. I think it should be UB to use a success error_code with expected<T, error_code>. Not having the value and having an error_code that represents success has no sense for me. The ongoing standard proposal doesn't say nothing about E and I don't know what it could say.
On 16/01/2018 10:18, Niall Douglas wrote:
To summarise what the mock up demonstrates:
1. error_category() gains a new virtual function called `bool failure(int code)` which is now called by error_code's operator bool.
As I hinted in another post but didn't quite actually state, rather than adding a new method, wouldn't it be sufficient to make operator bool do something like: return *this == error_condition(0, generic_category()); (Using generic_category() is more correct, but given the default constructor it might perhaps lead to less/zero code breakage to use system_category() instead.)
On Mon, Jan 15, 2018 at 1:18 PM, Niall Douglas via Boost < boost@lists.boost.org> wrote:
Myself and Peter feel this is worth fixing despite the increased runtime overhead. Others, including one of its original inventors, feel it is too much overhead.
I must ask once again how do we know it is "too much" overhead? Where is the real-world program which would be impossible or difficult to write otherwise? For some reason I never get an actual answer, and instead it's always something along the lines of "I'm an expert, trust me, I know". Well, I don't. Sorry. This applies to C++ exception handling and error handling in general. Yes, I know, there is overhead -- but where is the proof that 1) it matters, and 2) it's not worth it? Emil
Myself and Peter feel this is worth fixing despite the increased runtime overhead. Others, including one of its original inventors, feel it is too much overhead.
I must ask once again how do we know it is "too much" overhead? Where is the real-world program which would be impossible or difficult to write otherwise?
For some reason I never get an actual answer, and instead it's always something along the lines of "I'm an expert, trust me, I know". Well, I don't. Sorry.
A little unfair. I've supplied numbers to you before, but as with all such numbers, they're totally arbitrary and mostly meaningless. All anyone can really say is that with some real world application X, that Y was the case with that application. And that's about it. This particular debate comes down to how expensive is a virtual function call, and how frequently is code going to be doing `if(ec) ...`. As is fairly obvious by now, I consider virtual function call overhead to be pretty much in the stochastic noise of processor timings except on in-order CPUs [1]. As in, can be generally ignored as unimportant. I also think that `if(ec) ...` is not going to be called frequently, no more than twice per failing operation, and usually once. So to me, my proposed fix solves a real problem of correctness in lots of code out there in the wild which is currently subtly broken for virtually no cost. Therefore, it's a slam dunk. It should be implemented and the C++ standard upgraded to match. Obviously others disagree with me.
This applies to C++ exception handling and error handling in general. Yes, I know, there is overhead -- but where is the proof that 1) it matters, and 2) it's not worth it?
The "it matters" part depends on your attitude to the semantics behind observation of error codes. What does the code being 0 actually mean? Also, as Peter has explained several times now, composition of error_code returning functions in generic code where the category is unknown is hard without a generic facility to determine if some error_code means failure or not. For that alone, this change should be implemented. It breaks no existing code whatsoever, but fixes the semantics into what the programmer probably intended. Niall [1]: Of course raw benchmarks show virtual function calls are slower to no function call due to inlining in some artificial benchmark. But in real world code, the cost of a virtual function relative *to doing any work at all* is insignificant. While your doing work code is stalled on memory or whatever, the CPU is off busy executing virtual function calls and so on. Once you average it out, virtual function calls approach free in most real world code use cases on modern out-of-order CPUs. -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Tue, Jan 16, 2018 at 1:40 AM, Niall Douglas via Boost < boost@lists.boost.org> wrote:
Myself and Peter feel this is worth fixing despite the increased
runtime
overhead. Others, including one of its original inventors, feel it is too much overhead.
I must ask once again how do we know it is "too much" overhead? Where is the real-world program which would be impossible or difficult to write otherwise?
For some reason I never get an actual answer, and instead it's always something along the lines of "I'm an expert, trust me, I know". Well, I don't. Sorry.
A little unfair. I've supplied numbers to you before, but as with all such numbers, they're totally arbitrary and mostly meaningless.
I believe I was supporting your position in this case. If it fits the semantics, it should be a virtual function call unless there are problems with that, I was just pointing out that "but it's slow" is not a valid argument unless there is practical evidence to that effect.
This applies to C++ exception handling and error handling in general. Yes, I know, there is overhead -- but where is the proof that 1) it matters, and 2) it's not worth it?
The "it matters" part depends on your attitude to the semantics behind observation of error codes. What does the code being 0 actually mean?
By "it matters" I mean, what programs will be more difficult to write because the virtual function call is "too slow"? Just because this question is difficult or impossible to answer it doesn't mean it's unfair, because we have to evaluate the impact of a compromise. This can't be done in the abstract. Emil
On 01/16/18 00:18, Niall Douglas via Boost wrote:
That leaves the request to fix "if(ec) ..." which right now returns true if the value is 0, despite that much code writes "if(ec) ..." to mean "if error then ...". There is also the issue of error coding schemes not being able to have more than one success value, which usually must be 0. That's the remaining discussion point.
Question: is this still considered too much overhead?
I think the test you presented is rather optimistic in that it is comprised of a single translation unit. I think, in real applications the following are more common: - The error category is often implemented in a separate translation unit from the code that sets or tests for error codes with that category. This follows from the existing practice of declaring the category instance as a function-local static, where the function is defined in a separate TU. - The code that sets the error code is often in a separate TU than the code that tests for errors. This follows from the typical separation between a library and its users. Given the above, unless LTO is used, I think the compiler will most often not be able to optimize the virtual function call. I've converted your code to a synthetic benchmark, consisting of one header and two translation units (one with the test itself and the other one that defines the error category). The test still does not isolate the code of producing the error code from the code that analyzes it, so in that regard it is still a bit optimistic. I'm using gcc 7.2 and compiling the code with -O3. Here are the results on my Sandy Bridge CPU: Experimental test: 275565 usec, 362890788.017346 tests per second std test: 45767 usec, 2184980444.425023 tests per second This is a 6x difference. In the generated code I noticed that the compiler generated a check of whether the virtual function `failure` is actually `experimental::error_category::failure`. If it is, the code uses an inlined version of this function (otherwise, the actual indirect call is performed). So if you comment `code_category_impl::failure` the test succeeds and the indirect call is avoided. Here are the results for this case: Experimental test: 71711 usec, 1394486201.559036 tests per second std test: 48177 usec, 2075679266.039812 tests per second This is still a 1.5x difference. Now, I admit that this synthetic benchmark solely focuses on the one check for the error code value. The real applications will likely have much more code intervening the tests for error codes. The real world effect will be less pronounced. Still, I thought some estimate of the performance penalty would be useful, if only to show that this is not a zero overhead change. Do I think this overhead is significant enough? Difficult to tell. Certainly I'm not happy about it, but I could probably live with the 1.5x overhead. However, it still results in code bloat and there is no guarantee this optimization will be performed by the compiler (or that it will be effective if e.g. my code always overrides `error_category::failure`). Thing is, every bit of overhead makes me more and more likely consider dropping `error_code` in favor of direct use of error codes. `error_code` already carries additional baggage of the pointer to error category.
Andrey Semashev wrote:
In the generated code I noticed that the compiler generated a check of whether the virtual function `failure` is actually `experimental::error_category::failure`. If it is, the code uses an inlined version of this function (otherwise, the actual indirect call is performed). So if you comment `code_category_impl::failure` the test succeeds and the indirect call is avoided. Here are the results for this case:
Experimental test: 71711 usec, 1394486201.559036 tests per second std test: 48177 usec, 2075679266.039812 tests per second
This is still a 1.5x difference.
That's pretty good. In practice, if the function does something nontrivial, this amount of overhead will be entirely lost in the noise.
On 01/16/18 16:54, Peter Dimov via Boost wrote:
Andrey Semashev wrote:
In the generated code I noticed that the compiler generated a check of whether the virtual function `failure` is actually `experimental::error_category::failure`. If it is, the code uses an inlined version of this function (otherwise, the actual indirect call is performed). So if you comment `code_category_impl::failure` the test succeeds and the indirect call is avoided. Here are the results for this case:
Experimental test: 71711 usec, 1394486201.559036 tests per second std test: 48177 usec, 2075679266.039812 tests per second
This is still a 1.5x difference.
That's pretty good. In practice, if the function does something nontrivial, this amount of overhead will be entirely lost in the noise.
Probably, although I think this number is a bit optimistic because the branch predictor is trained by the loop and that the indirect call is disfavored (it's been moved out of the loop). My worry is that we may not even achieve this number if the compiler is not capable or for some reason not able to perform the optimization. The worst case overhead of 6x looks much more grim.
I think the test you presented is rather optimistic in that it is comprised of a single translation unit.
I deliberately did not use -fwhole-program. So the compiler only knew it was compiling to an executable not a shared library, but nothing more.
I think, in real applications the following are more common:
- The error category is often implemented in a separate translation unit from the code that sets or tests for error codes with that category. This follows from the existing practice of declaring the category instance as a function-local static, where the function is defined in a separate TU.
- The code that sets the error code is often in a separate TU than the code that tests for errors. This follows from the typical separation between a library and its users.
Given the above, unless LTO is used, I think the compiler will most often not be able to optimize the virtual function call.
As you noticed later in your reply, if a virtual function is marked final, the compiler inserts a check to see if the vtable's implementation matches and if so uses an inlined edition.
I've converted your code to a synthetic benchmark, consisting of one header and two translation units (one with the test itself and the other one that defines the error category). The test still does not isolate the code of producing the error code from the code that analyzes it, so in that regard it is still a bit optimistic.
I must protest at "optimistic". Your benchmark is actually the most pessimistic that it can be. You need to do some real work in there, let the out of order speculative execution do its thing. I usually throw in a FNV1a hash of 64 bytes, it's not much work, but is actual real work.
Do I think this overhead is significant enough? Difficult to tell. Certainly I'm not happy about it, but I could probably live with the 1.5x overhead. However, it still results in code bloat and there is no guarantee this optimization will be performed by the compiler (or that it will be effective if e.g. my code always overrides `error_category::failure`). Thing is, every bit of overhead makes me more and more likely consider dropping `error_code` in favor of direct use of error codes. `error_code` already carries additional baggage of the pointer to error category.
You might be interested to learn that in a real world code base I tested, padding error_code's size to 64 bytes produced no statistically observable slowdown on Intel Ivy Bridge. As soon as it tipped over 64 bytes though, it became very noticeable, about 5%. That was for error_code_extended in Outcome v1, I wanted to see how much payload I could cram in there. So when you say error_code comes with baggage over C error codes, I'll claim that you could in fact add lots more baggage still and probably see little effect. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Tue, Jan 16, 2018 at 7:50 AM, Niall Douglas via Boost < boost@lists.boost.org> wrote: <snip, discuss overhead of 'std::error_code' checks>
You might be interested to learn that in a real world code base I tested, padding error_code's size to 64 bytes produced no statistically observable slowdown on Intel Ivy Bridge. As soon as it tipped over 64 bytes though, it became very noticeable, about 5%. That was for error_code_extended in Outcome v1, I wanted to see how much payload I could cram in there.
So when you say error_code comes with baggage over C error codes, I'll claim that you could in fact add lots more baggage still and probably see little effect.
This is interesting: There's a long-recognized need for instance-specific error parameter-values (e.g., a "key-value-map" for parameters, such as to include the `file-name` that we we failed to open). This seems to be the "number-one" reason to create user-specific wrappers around `std::error_code`. So, adding an 'std::unique_ptr<>` member to `std::error_code` would allow that; and could also solve the memory-transport issue to remove `std::string` from the `std::error_code` interface. Recall that currently (on *all* platforms that I'm aware of), `sizeof(std::error_code) == sizeof(int)+sizeof(void*)`. ...so, this idea would add `sizeof(unique_ptr<>)` to `std::error_code`. --charley
AMDG On 01/16/2018 08:08 AM, charleyb123 . via Boost wrote:
This is interesting: There's a long-recognized need for instance-specific error parameter-values (e.g., a "key-value-map" for parameters, such as to include the `file-name` that we we failed to open). This seems to be the "number-one" reason to create user-specific wrappers around `std::error_code`.
So, adding an 'std::unique_ptr<>` member to `std::error_code` would allow that; and could also solve the memory-transport issue to remove `std::string` from the `std::error_code` interface.
unique_ptr would make error code move-only, which seems like too much of a breaking change. Also, it sounds like you're trying to re-invent Boost.Exception for error_code--why exactly are we avoiding exceptions, again?
Recall that currently (on *all* platforms that I'm aware of), `sizeof(std::error_code) == sizeof(int)+sizeof(void*)`.
...so, this idea would add `sizeof(unique_ptr<>)` to `std::error_code`.
In Christ, Steven Watanabe
On 01/16/18 17:50, Niall Douglas via Boost wrote:
Given the above, unless LTO is used, I think the compiler will most often not be able to optimize the virtual function call.
As you noticed later in your reply, if a virtual function is marked final, the compiler inserts a check to see if the vtable's implementation matches and if so uses an inlined edition.
Being final has no effect if the final function declaration is not immediately visible to the compiler. The compiler optimization that I described is performed regardless of that markup.
Do I think this overhead is significant enough? Difficult to tell. Certainly I'm not happy about it, but I could probably live with the 1.5x overhead. However, it still results in code bloat and there is no guarantee this optimization will be performed by the compiler (or that it will be effective if e.g. my code always overrides `error_category::failure`). Thing is, every bit of overhead makes me more and more likely consider dropping `error_code` in favor of direct use of error codes. `error_code` already carries additional baggage of the pointer to error category.
You might be interested to learn that in a real world code base I tested, padding error_code's size to 64 bytes produced no statistically observable slowdown on Intel Ivy Bridge. As soon as it tipped over 64 bytes though, it became very noticeable, about 5%. That was for error_code_extended in Outcome v1, I wanted to see how much payload I could cram in there.
So when you say error_code comes with baggage over C error codes, I'll claim that you could in fact add lots more baggage still and probably see little effect.
Sure, what you call "significant overhead" depends on what you actually do. That's why there is no single answer to the question you asked in the initial message. But note that `error_code` is supposed to be the replacement for all kinds of error codes in the wild. There are APIs that use error codes with very simple, even trivial functions like setters or getters. If we are to claim `error_code` is suitable for those cases then it really should be very *very* close in costs to a simple `int` to be viable. So if it, for instance, cannot fit in a register to return efficiently then that's a problem. If it requires an indirect call to check for success, that's a problem too.
If it requires an indirect call to check for success, that's a problem too.
Any function which would produce an error code will at best be in a userland library. At worst it will involve a context switch into the kernel. Can we really claim that the cost of (at worst) a simple indirect jump actually matters? However, I suspect this conversation will be fruitless. Too many entrenched positions and not enough evidence. Perhaps we should be looking forward. What is the semantic result of a system call in reality? *Either* error *or* result (or partial result with error, in the case where we supplied insufficient buffer space). Boost.Outcome seems to me to be going in the right direction here. Return a variant and visit that. No indirection, no if/else, no return values that contain "valid but unspecified" values. Simple, clean and difficult to abuse. On 16 January 2018 at 16:25, Andrey Semashev via Boost < boost@lists.boost.org> wrote:
On 01/16/18 17:50, Niall Douglas via Boost wrote:
Given the above, unless LTO is used, I think the compiler will most often not be able to optimize the virtual function call.
As you noticed later in your reply, if a virtual function is marked final, the compiler inserts a check to see if the vtable's implementation matches and if so uses an inlined edition.
Being final has no effect if the final function declaration is not immediately visible to the compiler. The compiler optimization that I described is performed regardless of that markup.
Do I think this overhead is significant enough? Difficult to tell.
Certainly I'm not happy about it, but I could probably live with the 1.5x overhead. However, it still results in code bloat and there is no guarantee this optimization will be performed by the compiler (or that it will be effective if e.g. my code always overrides `error_category::failure`). Thing is, every bit of overhead makes me more and more likely consider dropping `error_code` in favor of direct use of error codes. `error_code` already carries additional baggage of the pointer to error category.
You might be interested to learn that in a real world code base I tested, padding error_code's size to 64 bytes produced no statistically observable slowdown on Intel Ivy Bridge. As soon as it tipped over 64 bytes though, it became very noticeable, about 5%. That was for error_code_extended in Outcome v1, I wanted to see how much payload I could cram in there.
So when you say error_code comes with baggage over C error codes, I'll claim that you could in fact add lots more baggage still and probably see little effect.
Sure, what you call "significant overhead" depends on what you actually do. That's why there is no single answer to the question you asked in the initial message.
But note that `error_code` is supposed to be the replacement for all kinds of error codes in the wild. There are APIs that use error codes with very simple, even trivial functions like setters or getters. If we are to claim `error_code` is suitable for those cases then it really should be very *very* close in costs to a simple `int` to be viable. So if it, for instance, cannot fit in a register to return efficiently then that's a problem. If it requires an indirect call to check for success, that's a problem too.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman /listinfo.cgi/boost
On Mon, Jan 15, 2018 at 10:18 PM, Niall Douglas via Boost <boost@lists.boost.org> wrote:
Question: is this still considered too much overhead? Note that there is no longer ANY proposed breaking change with existing code. This is now 100% enhancement only.
Can't the is_error bool be stored in the ec object itself, at construction time? -- Olaf
On 01/16/18 18:56, Olaf van der Spek via Boost wrote:
On Mon, Jan 15, 2018 at 10:18 PM, Niall Douglas via Boost <boost@lists.boost.org> wrote:
Question: is this still considered too much overhead? Note that there is no longer ANY proposed breaking change with existing code. This is now 100% enhancement only.
Can't the is_error bool be stored in the ec object itself, at construction time?
Actually, I like this idea. I've modified my benchmark accordingly and it shows nearly identical performance as the current `std::error_code`: Experimental test: 253654 usec, 394237820.022550 tests per second Experimental2 test: 46353 usec, 2157357668.327832 tests per second std test: 45981 usec, 2174811335.116679 tests per second
2018-01-16 17:05 GMT+01:00 Andrey Semashev via Boost <boost@lists.boost.org> :
On 01/16/18 18:56, Olaf van der Spek via Boost wrote:
On Mon, Jan 15, 2018 at 10:18 PM, Niall Douglas via Boost <boost@lists.boost.org> wrote:
Question: is this still considered too much overhead? Note that there is no longer ANY proposed breaking change with existing code. This is now 100% enhancement only.
Can't the is_error bool be stored in the ec object itself, at construction time?
Actually, I like this idea. I've modified my benchmark accordingly and it shows nearly identical performance as the current `std::error_code`:
Experimental test: 253654 usec, 394237820.022550 tests per second Experimental2 test: 46353 usec, 2157357668.327832 tests per second std test: 45981 usec, 2174811335.116679 tests per second
This is now getting closer to Boost.outcome, isn't it? Regards, &rzej;
On 01/16/18 19:10, Andrzej Krzemienski via Boost wrote:
2018-01-16 17:05 GMT+01:00 Andrey Semashev via Boost <boost@lists.boost.org> :
On 01/16/18 18:56, Olaf van der Spek via Boost wrote:
Can't the is_error bool be stored in the ec object itself, at construction time?
Actually, I like this idea. I've modified my benchmark accordingly and it shows nearly identical performance as the current `std::error_code`:
Experimental test: 253654 usec, 394237820.022550 tests per second Experimental2 test: 46353 usec, 2157357668.327832 tests per second std test: 45981 usec, 2174811335.116679 tests per second
This is now getting closer to Boost.outcome, isn't it?
With my limited knowledge of Boost.Outcome, I don't think so. An outcome is supposed to carry a value or error, and `error_code` carries only the error code.
2018-01-16 17:15 GMT+01:00 Andrey Semashev via Boost <boost@lists.boost.org> :
On 01/16/18 19:10, Andrzej Krzemienski via Boost wrote:
2018-01-16 17:05 GMT+01:00 Andrey Semashev via Boost < boost@lists.boost.org> :
On 01/16/18 18:56, Olaf van der Spek via Boost wrote:
Can't the is_error bool be stored in the ec object itself, at construction time?
Actually, I like this idea. I've modified my benchmark accordingly and it shows nearly identical performance as the current `std::error_code`:
Experimental test: 253654 usec, 394237820.022550 tests per second Experimental2 test: 46353 usec, 2157357668.327832 tests per second std test: 45981 usec, 2174811335.116679 tests per second
This is now getting closer to Boost.outcome, isn't it?
With my limited knowledge of Boost.Outcome, I don't think so. An outcome is supposed to carry a value or error, and `error_code` carries only the error code.
Boost.Outcome provides a number of tools. One of types, `result<void>` (maybe error of value, but value is void), is exactly this: a `bool` flag and an `error_code`. Regards, &rzej;
On 01/16/18 19:25, Andrzej Krzemienski via Boost wrote:
2018-01-16 17:15 GMT+01:00 Andrey Semashev via Boost <boost@lists.boost.org> :
On 01/16/18 19:10, Andrzej Krzemienski via Boost wrote:
This is now getting closer to Boost.outcome, isn't it?
With my limited knowledge of Boost.Outcome, I don't think so. An outcome is supposed to carry a value or error, and `error_code` carries only the error code.
Boost.Outcome provides a number of tools. One of types, `result<void>` (maybe error of value, but value is void), is exactly this: a `bool` flag and an `error_code`.
Still, it is a different thing, semantically. If the `bool` is `true` (i.e. indicates value), it contains no error code, doesn't it? At least, I don't think it should. Also, a small technical detail. I believe carefully adding the flag to `error_code` doesn't increase its size. On 64-bit systems it fits nicely in the padding area. On 32-bit systems it could be merged into the category pointer, if we really wanted. Not sure `result<void>` does that.
Also, a small technical detail. I believe carefully adding the flag to `error_code` doesn't increase its size. On 64-bit systems it fits nicely in the padding area. On 32-bit systems it could be merged into the category pointer, if we really wanted. Not sure `result<void>` does that.
I've taken great care to ensure Outcomes fit into the ABI stack spill size (32 bytes) where possible. Layout is always: T value; unsigned flags; E error; So if T were an int and E an error_code, size on 64 bit would be 24 bytes (error_code is 16 bytes), but just 16 bytes on 32 bit. result<void> is also 24 bytes, rather than 20 bytes, due to the error_code containing a pointer forcing alignment to the next 8 byte multiple. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
2018-01-16 17:35 GMT+01:00 Andrey Semashev via Boost <boost@lists.boost.org> :
On 01/16/18 19:25, Andrzej Krzemienski via Boost wrote:
2018-01-16 17:15 GMT+01:00 Andrey Semashev via Boost < boost@lists.boost.org> :
On 01/16/18 19:10, Andrzej Krzemienski via Boost wrote:
This is now getting closer to Boost.outcome, isn't it?
With my limited knowledge of Boost.Outcome, I don't think so. An outcome is supposed to carry a value or error, and `error_code` carries only the error code.
Boost.Outcome provides a number of tools. One of types, `result<void>` (maybe error of value, but value is void), is exactly this: a `bool` flag and an `error_code`.
Still, it is a different thing, semantically. If the `bool` is `true` (i.e. indicates value), it contains no error code, doesn't it? At least, I don't think it should.
Yes you are correct. If there are any statuses representing success in producing a value, they will be discarded. Interestingly Outcome's `result` has the potential to represent both successfully computed value and a status code, without any additional spacial overhead. It was has now been disabled. But I know see how it could be useful. Regards, &rzej;
With my limited knowledge of Boost.Outcome, I don't think so. An outcome is supposed to carry a value or error, and `error_code` carries only the error code.
Actually the code() part of error_code is implicitly an outcome. Some code values are not errors, others are. It's just that error_code, being modelled on things like errno and GetLastError(), is implicitly archaic. If you'll forgive the expression, I feel we could do better than merely try to polish a turd from the 1950s. On 16 January 2018 at 17:15, Andrey Semashev via Boost < boost@lists.boost.org> wrote:
On 01/16/18 19:10, Andrzej Krzemienski via Boost wrote:
2018-01-16 17:05 GMT+01:00 Andrey Semashev via Boost < boost@lists.boost.org> :
On 01/16/18 18:56, Olaf van der Spek via Boost wrote:
Can't the is_error bool be stored in the ec object itself, at construction time?
Actually, I like this idea. I've modified my benchmark accordingly and it shows nearly identical performance as the current `std::error_code`:
Experimental test: 253654 usec, 394237820.022550 tests per second Experimental2 test: 46353 usec, 2157357668.327832 tests per second std test: 45981 usec, 2174811335.116679 tests per second
This is now getting closer to Boost.outcome, isn't it?
With my limited knowledge of Boost.Outcome, I don't think so. An outcome is supposed to carry a value or error, and `error_code` carries only the error code.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman /listinfo.cgi/boost
Andrzej Krzemienski wrote:
2018-01-16 17:05 GMT+01:00 Andrey Semashev via Boost <boost@lists.boost.org>:
On 01/16/18 18:56, Olaf van der Spek via Boost wrote:
Can't the is_error bool be stored in the ec object itself, at construction time?
Actually, I like this idea. I've modified my benchmark accordingly and it shows nearly identical performance as the current `std::error_code`:
Experimental test: 253654 usec, 394237820.022550 tests per second Experimental2 test: 46353 usec, 2157357668.327832 tests per second std test: 45981 usec, 2174811335.116679 tests per second
This is now getting closer to Boost.outcome, isn't it?
It's a conforming implementation of the same (implied so far) specification. You have bool error_code::failed() const noexcept; Returns: category().failed( value() ); Remarks: implementations are allowed to cache the result of the expression. Now, it's up to the implementation whether it will decide to cache the result in a bool or not. On a typical 64 bit platform, you have four bytes of unused padding between the `int value_` and the `error_category const * cat_` so you can fit a bool there (or 32 bools) without any size overhead. A 32 bit platform may choose to just do the virtual call each time instead. It's true that if we use Outcome, the "failed" bit is already there so we don't need to ask the error_code at all. But the topic here, brought up by Niall, is error_code and not Outcome. Presumably, being the author of Outcome, he wouldn't ask for something he knows he doesn't need. :-)
On 01/16/18 19:40, Peter Dimov via Boost wrote:
Andrzej Krzemienski wrote:
2018-01-16 17:05 GMT+01:00 Andrey Semashev via Boost <boost@lists.boost.org>:
On 01/16/18 18:56, Olaf van der Spek via Boost wrote:
Can't the is_error bool be stored in the ec object itself, at >> construction time?
Actually, I like this idea. I've modified my benchmark accordingly and > it shows nearly identical performance as the current `std::error_code`:
Experimental test: 253654 usec, 394237820.022550 tests per second Experimental2 test: 46353 usec, 2157357668.327832 tests per second std test: 45981 usec, 2174811335.116679 tests per second
This is now getting closer to Boost.outcome, isn't it?
It's a conforming implementation of the same (implied so far) specification. You have
bool error_code::failed() const noexcept;
Returns: category().failed( value() ); Remarks: implementations are allowed to cache the result of the expression.
For the record, I was thinking of (and testing) a different implementation that does not involve the `failed` call. Instead, I changed the `error_code` constructors: class error_code { int _value; bool _is_failure; const experimental::error_category *_category; public: constexpr error_code(int v, const experimental::error_category &cat) : _value(v), _is_failure(v != 0), _category(&cat) {} constexpr error_code(int v, bool is_failure, const experimental::error_category &cat) : _value(v), _is_failure(is_failure), _category(&cat) {} // Fixed operator bool, now asks category explicit operator bool() const noexcept { return _is_failure; } }; Calling (virtual) `error_category::failure` early is a waste if the user doesn't want to call that function himself. For instance, he may want to test specific error codes instead. Caching the result of `error_category::failure` on demand seems useless because most likely the error code will be tested for success only once. IMHO, this flag approach only makes sense when the flag is cheap and initialized early. For example, in the corresponding `make_error_code` overload. inline error_code make_error_code(my_error err) { return error_code(err, err < min_success || err > max_success, my_category); }
Andrey Semashev wrote:
Calling (virtual) `error_category::failure` early is a waste if the user doesn't want to call that function himself. For instance, he may want to test specific error codes instead.
It can still be worth it because it's more likely to occur in a context where the compiler can see what category is being used. When you have error_code ec( r, system_category() ); and system_category() is constexpr, the compiler knows with certainty what the call category().failed( r ) will do, so it will replace it with r != 0.
It can still be worth it because it's more likely to occur in a context where the compiler can see what category is being used. When you have
error_code ec( r, system_category() );
and system_category() is constexpr, the compiler knows with certainty what the call category().failed( r ) will do, so it will replace it with r != 0.
See f.ex. https://godbolt.org/g/6wGjCu
See f.ex. https://godbolt.org/g/6wGjCu
Yeah, except this doesn't work because now the constructor can't be constexpr. That's a core language defect IMO but what can you do. :-/
On 01/16/18 20:09, Peter Dimov via Boost wrote:
Andrey Semashev wrote:
Calling (virtual) `error_category::failure` early is a waste if the user doesn't want to call that function himself. For instance, he may want to test specific error codes instead.
It can still be worth it because it's more likely to occur in a context where the compiler can see what category is being used. When you have
error_code ec( r, system_category() );
and system_category() is constexpr, the compiler knows with certainty what the call category().failed( r ) will do, so it will replace it with r != 0.
This will only be possible if the implementation of the category is visible at the point of `error_code` construction. I'm assuming that typically it is not. If the category is supposed to be visible then there is no reason to make that method virtual. Make it a regular function and change the constructor like this: template< typename Category, enable_if_t< is_base_of_t< error_category, Category > >
constexpr error_code(int v, const Category &cat) : _value(v), _is_failure(cat.is_failure(v)), _category(&cat) {} Although, honestly, I don't see much point in having that function in the category at this point. The code should be more clean if it tested the code against the enum values instead of `int`. So instead of this: bool my_category::is_failure(int err) { return static_cast< my_error >(err) < my_error::min_success || static_cast< my_error >(err) > my_error::max_success; } I would prefer this: error_code make_error_code(my_error err) { return error_code(static_cast< int >(err), err < my_error::min_success || err > my_error::max_success, my_category); } Note that this approach allows to generate `error_code` objects with the same category from multiple different enums. I think, someone mentioned a use case for this earlier in the discussion - when multiple enums basically map onto the common error domain, which has a single error category.
For the record, I was thinking of (and testing) a different implementation that does not involve the `failed` call. Instead, I changed the `error_code` constructors:
I feel no love for this design. What I want to do is construct an error code from some C error code returned to me without additional logic. I should not need to know whether the C error code is success or failure. You've got to think of the many cases where we wrap C libraries like SQLite and we want to encapsulate its error/status coding system into C++ with semantic understanding of what those codes mean. So, specifically, their mapping onto std::errc, and their success/failure/warning meanings. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 01/16/18 22:00, Niall Douglas via Boost wrote:
For the record, I was thinking of (and testing) a different implementation that does not involve the `failed` call. Instead, I changed the `error_code` constructors:
I feel no love for this design. What I want to do is construct an error code from some C error code returned to me without additional logic. I should not need to know whether the C error code is success or failure.
Someone has to decide what code is a success and what is not. In my code, it is the creator of `error_code`, who is supposed to be familiar with the domain. It should usually be `make_error_code`, a facility coupled with the error category and error domain.
I feel no love for this design. What I want to do is construct an error code from some C error code returned to me without additional logic. I should not need to know whether the C error code is success or failure.
Someone has to decide what code is a success and what is not. In my code, it is the creator of `error_code`, who is supposed to be familiar with the domain. It should usually be `make_error_code`, a facility coupled with the error category and error domain.
That *might* be the case. But it absolutely might also not be. I'm not at all convinced that the constructor of an error_code will always know if that code represents success or failure. Moreover, I see no reason why it _should_ know. It's none of its business. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
It's true that if we use Outcome, the "failed" bit is already there so we don't need to ask the error_code at all. But the topic here, brought up by Niall, is error_code and not Outcome. Presumably, being the author of Outcome, he wouldn't ask for something he knows he doesn't need. :-)
Actually for Outcome, what I wanted from Boost.System was constexpr construction (got that) and removal of <string> (didn't get that). Outcome is useful precisely as a means to not have this problem with error_code. So the fix being discussed is not needed by Outcome. My interest in it is purely due to code written using Outcome which works with error codes. There is a second purpose though. Given my failure to remove <string>, I think it is now inevitable that I'll need to go implement a <system_error2> or something like that. It'll fix the header-only use problems and dragging in the STL allocator machinery. It'll also be more reliable for semantic comparisons. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Can't the is_error bool be stored in the ec object itself, at construction time?
Actually, I like this idea. I've modified my benchmark accordingly and it shows nearly identical performance as the current `std::error_code`:
Experimental test: 253654 usec, 394237820.022550 tests per second Experimental2 test: 46353 usec, 2157357668.327832 tests per second std test: 45981 usec, 2174811335.116679 tests per second
Are you still calling the virtual function, but now in the constructor? I didn't see you post any source code anywhere. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 01/16/18 21:41, Niall Douglas via Boost wrote:
Can't the is_error bool be stored in the ec object itself, at construction time?
Actually, I like this idea. I've modified my benchmark accordingly and it shows nearly identical performance as the current `std::error_code`:
Experimental test: 253654 usec, 394237820.022550 tests per second Experimental2 test: 46353 usec, 2157357668.327832 tests per second std test: 45981 usec, 2174811335.116679 tests per second
Are you still calling the virtual function, but now in the constructor?
No, the flag is initialized in the `error_code` constructor. By default it is inferred from the error code value, but the user can specify the flag explicitly.
I didn't see you post any source code anywhere.
I posted an excerpt later in the discussion. For convenience, I'm attaching the full code here.
participants (17)
-
Andrey Semashev -
Andrzej Krzemienski -
charleyb123 . -
Chris Glover -
Emil Dotchevski -
Gavin Lambert -
James E. King, III -
Nevin Liber -
Nevin Liber -
Niall Douglas -
Olaf van der Spek -
Paul A. Bristow -
Peter Dimov -
Richard Hodges -
Robert Ramey -
Steven Watanabe -
Vicente J. Botet Escriba