Skip to content

refactor: Apply override specifier consistently#18914

Merged
maflcko merged 2 commits intobitcoin:masterfrom
hebasto:200508-override
May 11, 2020
Merged

refactor: Apply override specifier consistently#18914
maflcko merged 2 commits intobitcoin:masterfrom
hebasto:200508-override

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented May 8, 2020 •
edited
Loading

Two commits are split out from #16710 to make reviewing easier.

From C++ FAQ:

C.128: Virtual functions should specify exactly one of virtual, override, or final
Reason Readability. Detection of mistakes. Writing explicit virtual, override, or final is self-documenting and enables the compiler to catch mismatch of types and/or names between base and derived classes. However, writing more than one of these three is both redundant and a potential source of errors.

@hebasto hebasto mentioned this pull request May 8, 2020
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented May 8, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced May 8, 2020
Closed
Closed
Closed
Closed
Merged
@practicalswift
Copy link
Contributor

practicalswift commented May 9, 2020

ACK d044e0e: consistent use of override prevents bugs + patch looks correct + Travis happy

@maflcko
Copy link
Member

maflcko commented May 9, 2020 •
edited
Loading

ACK d044e0e, based on my understanding that adding override or final to a function must always be correct, unless it doesn't compile!?

@vasild
Copy link
Contributor

vasild commented May 11, 2020

ACK d044e0e

@MarcoFalke that is my understanding too, except that if override is used when nobody is further overriding a given method then it is better to use final instead of override. This will compile. Even in this case it is better to have override instead of nothing.

@sipa
Copy link
Member

sipa commented May 11, 2020

It's my understanding that changing final/override can never affect semantics, only potentially change whether something compiles or not.

In addition, override imples virtual, and final implies override, so it should never be needed to specify more than one of these 3.

@maflcko maflcko merged commit eb2ffbb into bitcoin:master May 11, 2020
@hebasto hebasto deleted the 200508-override branch May 11, 2020 17:41
@ryanofsky ryanofsky mentioned this pull request Jun 5, 2020
Merged
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 28, 2021
@hebasto @PiRK
refactor: Apply override specifier consistently
4770199 
Summary:
refactor: Use override for non-final overriders
1551cea2d52cac403ff506a7cc955d8de8fd6f3e
refactor: Remove override for final overriders
d044e0ec7d37bbcdf10bbdb903b9119741c7297d

> Two commits are split out from [[bitcoin/bitcoin#16710 | PR16710]] to make reviewing easier.
>
> From C++ FAQ:
>
>    C.128: Virtual functions should specify exactly one of virtual, override, or final
>     Reason Readability. Detection of mistakes. Writing explicit virtual, override, or final is self-documenting and enables the compiler to catch mismatch of types and/or names between base and derived classes. However, writing more than one of these three is both redundant and a potential source of errors.

This is a backport of Core [[bitcoin/bitcoin#18914 | PR18914]]

Most of the work was done already in D767

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9082
@PastaPastaPasta PastaPastaPasta mentioned this pull request Jun 15, 2021
Merged
kwvg added a commit to kwvg/dash that referenced this pull request Jun 17, 2021
@kwvg
bitcoin#18914: Apply override specifier consistently
73f9fcb
UdjinM6 added a commit to dashpay/dash that referenced this pull request Jun 23, 2021
@UdjinM6
Merge pull request #4196 from kittywhiskers/override
988fa6a 
merge bitcoin#18914, bitcoin#13306, bitcoin#16424, bitcoin#13899, bitcoin#17486, bitcoin#17880, bitcoin#18145, bitcoin#18843, bitcoin#16710: split warnings out of CXXFLAGS, add more flags
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@hebasto @DrahtBot @practicalswift @maflcko @vasild @sipa @fanquake