Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should the library use override, instead of virtual, in derived classes? #751

Closed
AlisdairM opened this issue Jun 11, 2016 · 2 comments
Closed

Comments

@AlisdairM
Copy link
Contributor

I believe the use of the override (contextual) keyword, rather than virtual, when declaring an overriding function in a class definition, is purely a matter of style, not semantics. As such, it should be addressed (if at all) as an editorial issue.

I believe users of more recent dialects of C++ would expect this syntax, and we should actively make such a change - and will prepare a pull request asap if the editors show interest.

There are two main scenarios:

  1. deriving from a regular class, such as bad_exception::what overriding exception::what.
    such cases are relatively straight forward, and should pose no concerns

  2. overriding a virtual function in a dependent base class, such as
    basic_filebuf<CHAR, TRAITS>::overflow overriding basic_streambuf<CHAR, TRAITS>::overflow.

The concern here is that a user specialization might not provide the virtual function, so adding override becomes a diagnosable error, and such a change might merit LWG approval via the issues process. However, the user is constrained that their specialization must implement the interface of the primary template, so they have always been required to provide that virtual function with exactly that signature, so I claim this can still be reasonably handled editorially.

There are no cases in the library of a putative case 3, where a type derives directly from a template parameter, where it would be entirely unknown what virtual function the parameterizing class would have. If there are such cases and I have missed them, use of override would clearly be non-editorial.

@zygoloid
Copy link
Member

I agree both that this is a good idea (the library synopses should use idiomatic modern C++), and editorial. @mclow, do you want to run this past the lib- reflector before we go ahead with it? We can easily back it out if there turns out to be reasonable opposition.

There are no cases in the library of a putative case 3, where a type derives directly from a template parameter, where it would be entirely unknown what virtual function the parameterizing class would have. If there are such cases and I have missed them, use of override would clearly be non-editorial.

Yes, but only for those cases, so if you find any when making this change, you can just skip those ones :)

@mclow
Copy link
Contributor

mclow commented Jun 13, 2016

I'm OK with this as well. While I would think that running it by the -lib reflector would normally be prudent, I think Richard's two points of (a) editorial, and (b) easily reverted are enough.

I think a "we made this change" to the -lib reflector (i.e, post-commit review) will be fine.

FrankHB pushed a commit to FrankHB/draft that referenced this issue Jul 9, 2016
This patch makes consistent use of the 'override' contextual
keyword for every member function (other than destructors)
that overrides a virtual function in one of its base classes.

In general, this came down to one of three cases:
1) the 'what' function for an exception class
2) the allocation functions of a memory resource
3) the implementation methods of a stream buffer.

As far as I can tell, all other virtual function declarations
in the standard library are intended as customization points
for users, and are not overridden in the standard itself.

Fixes cplusplus#751
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants