Use the override specifier (C++11) where we expect to be overriding the virtual function of a base class.
Rationale: By using the override specifier (C++11) we're guaranteed a compile-time error if our stated override expectation is not fulfilled. This avoids gotchas with regards to function renames, function signature changes, etc. Additionally it clarifies the intended behaviour for the reader.
Nice! Is there an automated way I can check that you didn't miss any, or did you do it by hand?
-Wsuggest-override should do it, though google says it may be inconsistent wrt 'final'.
https://github.com/theuni/bitcoin/commit/9b1f7626a4c16924d0921eb822efc40f77212f8c adds those suggested by gcc. Note that I just blindly added them, didn't verify correctness.
There are a bunch more in qt, even ignoring protobufs.
Concept ACK, but without being able to enforce, I fear that this might turn into another -Wshadow that drove @laanwj crazy.
@theuni Can you give some context for the -Wshadow incident?
@practicalswift Different compilers and different versions of the same compiler have non-matching notions of what "shadow" means. Some of them even had false positives. Effectively it was impossible to enforce this rule when the code was written by a developer and we had weekly cleanup pulls to "fix wshadow warnings". Let's avoid that this turns into a avalanche of "add override specifier" pulls.
@MarcoFalke Agreed! Just out of curiosity, has anyone tried to add proper override specifiers (project wide) before?
@TheBlueMatt I believe the current version is 100 % complete with the exception of the src/qt/ and src/univalue/ directories which I omitted intentionally. Please let me know if you find any counterexamples – cases which are not marked override in this PR but for which override should be used.
utACK d39888d178fd72235c2aeedb97d3b7f99260da38
As adding override can never cause a behaviour change, utACK-because-it-compiles d39888d178fd72235c2aeedb97d3b7f99260da38
@theuni Can you give some context for the -Wshadow incident?
Discussion started here: #8105 (comment) As for the fallout: https://github.com/bitcoin/bitcoin/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aclosed%20wshadow https://github.com/bitcoin/bitcoin/pulls?q=is%3Apr+wshadow+is%3Aclosed
It got increasingly heated over time, until we finally decided to hack the warning out again.
Concept ACK, but without being able to enforce, I fear that this might turn into another -Wshadow that drove @laanwj crazy.
Do compilers agree on when override should be used?
We have a few uses of override in the code since #10550 and I don't think it has given any issues.
I thnk I've only seen warnings from when override was applied to part of the methods instead of the entire class. Clang does this.
I don't think this can be as bad as Wshadow, where different versions of compilers had different interpretations. Also new classes aren't introduced that frequently.
So I think it's ok...
@laanwj I'm not worried about adding overrides, I definitely think we should!
I was only suggesting that because we can't enforce them with static analysis, we'll occasionally forget them and end up with one-off PRs like this one. Which is fine by me :)
clang/gcc both have warnings that can be enabled, but sadly we can't use them by default because:
Apparently gcc still warns about missing override when 'final' is used instead
From what I understand, override marks a method as being overridden. final marks it as non-overridable. As they work in the opposite direction through the inheritance hierarchy I don't see why one would be a replacement for the other?
Our dependencies don't necessarily use them, so we're flooded with warnings for qt/protobufs/leveldb.
That's a good point. Let's indeed not enable those warnings by default, just try to use it in our own code where we can (as this PR does).
@sipa I thought the value of adding override is catching some bugs that would arise through renames. As a result blinding adding it wouldn't be a great idea, even though it can never cause a change in behavior because doing so would undermine the advantage of it, no?
@gmaxwell I believe @sipa meant the fact that it compiles is good enough because 'override' triggers an error if it doesn't actually override anything. If the function signature ever changes (on accident), it will no longer override its base, and will fail to compile.
I looked these up to make sure that they're actual errors rather than being left up to compilers to decide:
§10.3.5
If a virtual function is marked with the virt-specifier override and does not override a member function of a base class, the program is ill-formed.
§9.2.9:
A virt-specifier-seq shall contain at most one of each virt-specifier. A virt-specifier-seq shall appear only in the declaration of a virtual member function.
Needs rebase.
Rebased! :-)