[system] virtual ~error_category
The virtual destructor of error_category creates problems for constexpr-ification because it makes the class non-literal. I can't think of a scenario in which we need the destructor to be virtual. It's probably virtual for "guideline" reasons - the class has virtual functions. But does it really need to be? Error categories aren't destroyed via a pointer to base. They are, in fact, static instances, not destroyed at all, except on program exit. Can anyone think of a case in which error_category needs the virtual destructor?
-----Original Message----- From: Boost [mailto:boost-bounces@lists.boost.org] On Behalf Of Peter Dimov via Boost Sent: Montag, 12. Februar 2018 20:13 Subject: [boost] [system] virtual ~error_category
Can anyone think of a case in which error_category needs the virtual destructor?
I don't know of any reason, but I'm also not really proficient in Boost.System. I would like to suggest to make the dtor protected though if you make it non-virtual. That would lessen the chance of something going wrong. E.g. user-code may contain strange code constructing and destructing error categories for whatever reason. (Of course the dtors of all pre-defined non-final categories should then be made non-virtual protected too.) Regards, Paul
On Mon, 2018-02-12 at 21:13 +0200, Peter Dimov via Boost wrote:
The virtual destructor of error_category creates problems for constexpr-ification because it makes the class non-literal.
I can't think of a scenario in which we need the destructor to be virtual. It's probably virtual for "guideline" reasons - the class has virtual functions. But does it really need to be? Error categories aren't destroyed via a pointer to base. They are, in fact, static instances, not destroyed at all, except on program exit.
Can anyone think of a case in which error_category needs the virtual destructor?
None at all.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo. cgi/boost
On 12/02/2018 19:13, Peter Dimov via Boost wrote:
The virtual destructor of error_category creates problems for constexpr-ification because it makes the class non-literal.
I can't think of a scenario in which we need the destructor to be virtual. It's probably virtual for "guideline" reasons - the class has virtual functions. But does it really need to be? Error categories aren't destroyed via a pointer to base. They are, in fact, static instances, not destroyed at all, except on program exit.
Can anyone think of a case in which error_category needs the virtual destructor?
The same idea (removing the virtual destructor) came up recently on SG14
for the proposed `status_domain` which is the proposed v2 of
`
But as far as Boost.System goes, I think de-virtualising its destructor is a major API break. I can see plenty of code out there in the wild relying on that destructor firing when it's supposed to, and Boost devirtualising it would equal a resource leak where there was none before. That's as big as break as changing what `if(ec)` means (which BTW is currently being deleted in `status_code` to force people to write what they actually mean).
It also occurs to me now that Charley mentioned to me once that some people instantiate custom categories carrying payload and hand them out one per error_code instance. It's a way of attaching payload to error_code. That use case definitely requires a virtual destructor to work right. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Niall Douglas wrote:
It also occurs to me now that Charley mentioned to me once that some people instantiate custom categories carrying payload and hand them out one per error_code instance. It's a way of attaching payload to error_code. That use case definitely requires a virtual destructor to work right.
~error_code does not destroy the category, so the virtuality of the destructor makes no difference. The category will leak in either case. This is also not quite conforming for another reason; the category is supposed to be a singleton and two instances should compare equal only when they are the same object (per the standard).
It also occurs to me now that Charley mentioned to me once that some people instantiate custom categories carrying payload and hand them out one per error_code instance. It's a way of attaching payload to error_code. That use case definitely requires a virtual destructor to work right.
~error_code does not destroy the category, so the virtuality of the destructor makes no difference. The category will leak in either case.
Not if the error code's destructor deletes its category.
This is also not quite conforming for another reason; the category is supposed to be a singleton and two instances should compare equal only when they are the same object (per the standard).
People wrote code assuming the category destructor is virtual. It is right now both in Boost.System and the STL. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Not if the error code's destructor deletes its category.
Well it doesn't.
You misunderstand me. I'm saying that there are people who make error categories via a factory, one per payload carried, and they attach them to a custom error_code whose destructor cleans up the bespoke error category. At least, this is the gist of what Charley told me. I could be wrong, memory could be faulty. I'm the wrong person to say on this. Maybe Charley will pipe in? I'd also loop in Chris on this. You might remember he said he's got custom categories retaining state back when he was arguing against fixing `if(ec)`. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Mon, Feb 12, 2018 at 1:36 PM, Niall Douglas via Boost < boost@lists.boost.org> wrote:
<snip>, I'm saying that there are people who make error categories via a factory, one per payload carried, and they attach them to a custom error_code whose destructor cleans up the bespoke error category.
At least, this is the gist of what Charley told me. I could be wrong, memory could be faulty. I'm the wrong person to say on this. Maybe Charley will pipe in?
In my implementation, the 'error_code' is merely a "handle" to an error-instance-with-field-values managed within the 'error_category'. Specifically: 'int error_code::value()' is the "handle" within the 'error_category' that does all the resource management. So, I don't need any behavior in 'error_code::dtor()', because I'm assuming many arbitrary-lifespan copies (and the 'error_category' is robust in the face of stale handle lookups). But, I *could* see how one might do trickery in 'error_code::dtor' (that just seems really expensive, since 'error_code' has value semantics, so I'd assume people don't do that). But, we're all really just experimenting anyway. I'd also loop in Chris on this. You might remember he said he's got
custom categories retaining state back when he was arguing against fixing `if(ec)`.
I'm curious about any/all use cases where people put custom-state in the derived 'error_category'. I did that, but I'm not excited about it. In my next design, all custom state goes elsewhere, except I *might* consider the derived 'error_category' having a handle to custom state (managed by somebody else). --charley
Niall Douglas wrote:
You misunderstand me.
I'm saying that there are people who make error categories via a factory, one per payload carried, and they attach them to a custom error_code whose destructor cleans up the bespoke error category.
Even if you only use your own custom error codes, you still need either to reference count the category, or to clone it in the copy constructor. You can't just delete it blindly because you don't know how many copies of the error code have been done. And either approach requires you to store not error_category* but your_error_category*, where your_error_category is a base class that derives from error_category. This _is_ silent breakage though as ~your_error_category was virtual by inheritance before and no longer will be. Not how error_category was supposed to be used but what can you do. If it's there, people will take advantage of it. :-/
On Mon, Feb 12, 2018 at 12:50 PM, Niall Douglas via Boost < boost@lists.boost.org> wrote:
But as far as Boost.System goes, I think de-virtualising its destructor is a major API break. I can see plenty of code out there in the wild relying on that destructor firing when it's supposed to, and Boost devirtualising it would equal a resource leak where there was none before. That's as big as break as changing what `if(ec)` means (which BTW is currently being deleted in `status_code` to force people to write what they actually mean).
It also occurs to me now that Charley mentioned to me once that some people instantiate custom categories carrying payload and hand them out one per error_code instance. It's a way of attaching payload to error_code. That use case definitely requires a virtual destructor to work right.
Yes, I did an implementation that effectively stored in the
(derived-)error_category all the state associated with a (circular-buffer)
of error-instances with instance-specific 'field-values'. This worked.
However:
(1) It's kind of awkward, and I'm not sure I'm excited about this design
approach. It was simply the only way I could figure out how to handle
stable error-field-values with the current
So, I'm fine with dropping the 'virtual dtor' for 'error_category'.
Ok, cool. For the record, proposed `status_code` is templated and thus can carry arbitrary payload, thus solving that problem in `error_code`. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On 13/02/2018 09:40, Niall Douglas wrote:
So, I'm fine with dropping the 'virtual dtor' for 'error_category'.
Ok, cool. For the record, proposed `status_code` is templated and thus can carry arbitrary payload, thus solving that problem in `error_code`.
Bear in mind that templating can itself be a barrier to usage, especially when people want to keep implementation in cpp files rather than making everything into templates. (I'm not saying that templating is the wrong choice, but this was probably one of the motivations for current error_code not being a template.)
Ok, cool. For the record, proposed `status_code` is templated and thus can carry arbitrary payload, thus solving that problem in `error_code`.
Bear in mind that templating can itself be a barrier to usage, especially when people want to keep implementation in cpp files rather than making everything into templates.
(I'm not saying that templating is the wrong choice, but this was probably one of the motivations for current error_code not being a template.)
Both status_code and status_code_domain come in type erased and templated forms. If you use the templated form, the optimiser eliminates all runtime overhead in recent GCCs and clangs, even down to eliminating instantiating the domain singleton because it's a constexpr variable. If you use the type erased form, you do incur the indirect branch overhead unless you turn on LTO. So, you're covered. Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
On Mon, Feb 12, 2018 at 11:45 AM, Niall Douglas via Boost < boost@lists.boost.org> wrote:
Some on SG14 felt "so what?" under the assumption that error categories are surely never destroyed?
No, that's under the assumption that the game will reboot to load the next level anyway. :)
Niall Douglas wrote:
It's very valid for a custom error code category to allocate some sort of internal state e.g. a hash table for looking up messages. This could be allocated statically, but it might also be allocated by the category instance on the basis that it was until now the same thing. This would not be freed on destruction if the `error_category` destructor were not virtual.
That's not true in the cases I can think of. Could you illustrate with code?
But as far as Boost.System goes, I think de-virtualising its destructor is a major API break.
It certainly is a potentially breaking change.
I can see plenty of code out there in the wild relying on that destructor firing when it's supposed to, and Boost devirtualising it would equal a resource leak where there was none before.
Yes, it's better to make it protected, as Paul Groke observed. This way there'll be no silent failures.
participants (7)
-
charleyb123 . -
Emil Dotchevski -
Gavin Lambert -
Groke, Paul -
Niall Douglas -
Peter Dimov -
Richard Hodges