[system] virtual ~error_category

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[system] virtual ~error_category

Boost - Dev mailing list
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?


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
> -----Original Message-----
> From: Boost [mailto:[hidden email]] 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


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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
`<system_error>`.

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.

Some on SG14 felt "so what?" under the assumption that error categories
are surely never destroyed? But then I reminded people of dynamic
unloading of shared libraries.

We then came up with a solution for `status_domain` whereby its
`message()` is going to return an atomic reference counted holder which
is a shared_ptr in all but name (but not one of those). That way
lifetime can be tracked, and the "right thing (TM)" can be done on
dynamic shared library unload. We can also drop the virtual destructor,
and make the `status_domain` completely constexpr apart from the member
functions it has which can't be.

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).

Niall

--
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
> 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/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Feb 12, 2018 at 11:45 AM, Niall Douglas via Boost <
[hidden email]> 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. :)

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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).


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Feb 12, 2018 at 12:50 PM, Niall Douglas via Boost <
[hidden email]> 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 <system_category> design (not
tied to 'error_code' value semantics).

(2) It might be "improper" to not have a virtual-dtor; But really, even if
I kept this current design I wouldn't care:  Because all log / filter /
pruning / sampling occurs as error instances are created-and-accessed (so
we probably don't really care about whether resources are deleted from an
explicit 'error_category::dtor()')

(3) The current design kinda demands the "singleton" behavior of
'error_category' (although I've found at least a couple work-arounds from
that which I believe to be conforming).  So, my guess is that people really
aren't expecting the 'error_category::dtor()' to actually do any "real"
work (since it probably only executes upon process-terminate, unless the
user is doing something really weird, which I admit is possible.)

So, I'm fine with dropping the 'virtual dtor' for 'error_category'.

--charley

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
>> 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/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
Niall Douglas wrote:

> Not if the error code's destructor deletes its category.

Well it doesn't.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
>> 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/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> 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/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Feb 12, 2018 at 1:36 PM, Niall Douglas via Boost <
[hidden email]> 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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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. :-/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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.)


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [system] virtual ~error_category

Boost - Dev mailing list
>> 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/


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost