Add final where appropriate #9545

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:add-overrides-where-appropriate changing 13 files +34 −34

practicalswift commented at 7:28 PM on January 13, 2017: contributor

No description provided.
theuni commented at 7:32 PM on January 13, 2017: member

concept ACK, but please prefer final where possible
Add final where appropriate 9d847d2474
practicalswift force-pushed on Jan 13, 2017
practicalswift renamed this:
~~Add override:s where appropriate~~
Add final where appropriate
on Jan 13, 2017
practicalswift commented at 8:04 PM on January 13, 2017: contributor

@theuni Done :-)
fanquake added the label Refactoring on Jan 14, 2017
MarcoFalke commented at 11:37 AM on January 14, 2017: member

same binaries on 9d847d2474a229f7a10d6e5fffd433aba7ce85cc, fyi

in src/test/coins_tests.cpp:None in 9d847d2474

  27 | @@ -28,7 +28,7 @@ class CCoinsViewTest : public CCoinsView
  28 |      std::map<uint256, CCoins> map_;
  29 |  
  30 |  public:
  31 | -    bool GetCoins(const uint256& txid, CCoins& coins) const
  32 | +    bool GetCoins(const uint256& txid, CCoins& coins) const final

sipa commented at 5:51 PM on January 14, 2017:

Isn't it easier to mark the entire class CCoinsViewTest as final?

sipa commented at 6:10 PM on January 14, 2017: member

I verified that with GCC 6.2 at -O2, a final on either a method or a class can avoid a vtable lookup, so concept ACK for sure.

I don't think we have a need for specifying final on a per-method bases, though - all examples you are applying this to can use class finals.

Other classes: CCoinsViewCache (coins.h), CCoinsViewDB (txdb.h), CCoinsViewMempool (txmempool.h), CWallet (wallet/wallet.h).
morcos commented at 10:36 PM on January 15, 2017: member

@sipa how does that mesh with @marcofalke's observation of same binaries? Different gcc?

I'd prefer if we decide to do this that we add some documentation that it's just for performance reasons and that if people change the code so there are non obvious requirements for a derived class that they should document that. It seems to me final is sometimes used to indicate don't footgun yourself by trying to derive from this.
theuni commented at 3:34 PM on January 16, 2017: member
@morcos Agreed, and I think that a comment at the location of the "final" in that case would suffice. For ex:
```
class Derived : Base
{
    foo() final // unsafe for derivation. Do not change to 'override'.
};

class Fickle final // deriving from Fickle would be unsafe.
{
};
```
There's no way for me to override foo() or Fickle without removing those warnings. If nothing else, it would be painfully obvious in review, and a change would have to be paired with a justification.
sipa commented at 4:00 PM on January 16, 2017: member

@morcos I only tested with isolated example, where class B derives from class A, and an overridden method is called on a variable of type B that comes from another module. As the compiler can't know that B isn't a reference to an object of a class C that derives from B, it has to emit the vtable lookup. But in many cases, analysis can show that this B variable points to an object of type B exactly (especially if it was constructed within the same module). If adding final does not change the binary, I guess that for all cases where it was introduced, the compiler was already able to infer enough. Note that there are more classes were final could apply.
laanwj commented at 9:56 AM on April 26, 2017: member

Let's not just randomly pepper final behind method signatures because we can.

There's no way for me to override foo() or Fickle without removing those warnings. If nothing else, it would be painfully obvious in review, and a change would have to be paired with a justification.

This sounds like a better use of final. Document why on a per-case basis. And prefer doing it per class instead of for every single method.
practicalswift closed this on Apr 26, 2017
practicalswift deleted the branch on Apr 10, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels