Breaking change that shouldn't break anything! ('final' keyword)

General JUCE discussion
1

I’ve just pushed a bunch of changes to develop where we’ve added “final” declarations to some classes which really shouldn’t be inherited!

It’s inevitable that this will break code for some people who’ve done silly things, because it’s a really common beginner mistake to over-use inheritance. But we all think this is really important because

  • It’ll highlight some bad choices for people whose code gets broken, and should hopefully be a useful learning experience for them to rewrite it using composition rather than inheritance!
  • It’ll prevent new beginners from misusing these things in the first place

However, I’ve been a little bit conservative about some classes, not adding the flag to things like Array that may be used as base classes for advanced and cunning purposes (e.g. empty base class optimisation etc).
So if this breaks something where you really think it gets in the way of a genuine, sensible use-case, then please let us know and if your argument is persuasive, then we may relent in some cases!

6 Likes
Image declared as 'final' -?
Algorithm to Obtain Destination File in moveFileTo()
2

Is there some reason to ever inherit AudioBuffer? I noticed that’s missing from the finalized classes in the commit.

3

oh good point. (Though I bet a few people have done!)

Yes, I haven’t gone through the whole library yet, just started with a batch from the core modules. We’ll probably gradually keep adding classes as we go along, but do shout if you see any you’d really like to see locked down!

4

I haven’t done it … but to pick a controversial example … what’s fundamentally terrible about something like:

class ControversalAudioBuffer : public AudioBuffer<float> { public: double sampleRate; };

5

Wouldn’t pass a code-review here… Would encourage composition instead:

struct BufferAndSampleRate
{
    AudioBuffer<float> buffer;
    double sampleRate;
};
1 Like
6

i.e. prefer composition.

But I’m curious as to the reason. There’s no real benefit to the inheritance approach (unless less typing counts), but I can’t put my finger on the danger either in this case.

1 Like
7

The main danger is from slicing if you try to copy these classes, or pass them to functions that expect the base class and hence mess up.

2 Likes
8

Revisiting this topic.

File is declared as final. Why this is an issue? I have a use case for integrating encryption/decryption of files on the fly for a secure environment with a requirement for no plaintext data ever hitting backing storage.

With ‘File’ declared as final my design choices are limited. The most obvious one is to produce an XFile class (could not resist) that I can use wherever I need file and delegate to an internal file object. The only new methods I need are ::createXInputStream -> std::unique_ptr and similar for ::outputStream. Now I either re-implement all the delegation methods for all other uses of File, or I implement a simplified XFile class which has a method ::getFile() to allow me direct access to all the other file methods. Either way this is a lot of code for a simple change.

Without the Final declaration for file this is one whole lot simpler. I simply inherit XFile from file, add the ::createXInputStream and ::createXOutputStream methods and the derived implementations of FileInputStream and FileOutputStream attending these. Comparatively simple - and then all I need do is replace File with XFile where the encryption support is required.

I estimate removing the ‘Final’ keyword in this instance will save me a sizeable amount of coding and testing, and will yield a cleaner implementation.

“Can I have inheritance back please mister?”

9

I don’t think this is a good use case for inheritance. If the only thing you need are two factory functions, that could probably be static anyway, you can simply put them in your own namespace. Doesn’t even need to be a class or struct:

namespace CryptFile
{
    static inline std::unique_ptr<juce::InputStream> createReadStream (juce::File f) { ... }
    static inline std::unique_ptr<juce::OutputStream> createWriteStream (juce::File f) { ... }
}
1 Like
10

Thanks for that Daniel - I think you have nailed the core of what I need in a very concise way.

Okay - claim for removing ‘final’ retracted. I will work out an implementation based on this model.

Sometimes all that is needed is a second pair of clear eyes. Thank you.

11

Agreed, although I wouldn’t recommend static inline on functions in headers at namespace scope. static informs the linker that you want a separate copy of the function in each translation unit which includes the function definition, which is probably not what you want. Just inline will work fine, and will also give the linker the go-ahead to remove duplicate definitions.

2 Likes
12

Nod. Clear. Also this is not good candidate code for inlining IMO.

13

Wouldn’t removing the inline keyword in a header implementation result in an “unused function” warning? It won’t be used in every TU…
The inline keyword doesn’t really result in inlined code any more. The linker won’t take those kind of commands from users any longer…

https://en.cppreference.com/w/cpp/language/inline

Since this meaning of the keyword inline is non-binding, compilers are free to use inline substitution for any function that’s not marked inline, and are free to generate function calls to any function marked inline. Those optimization choices do not change the rules regarding multiple definitions and shared statics listed above.

14

Sorry, I’m suggesting to only use inline and to remove the static.

15

Oh I see, sorry…
I wonder why I learned many summers past to use static in that instance…
Good to know it’s not needed.

1 Like
16

I use “static inline” in C++ code that can be compiled in C also. The rules are more complex in that cases < https://www.greenend.org.uk/rjk/tech/inline.html >. I remember to had duplicate copies of the object code if not marked static (don’t know if it always the case nowaday). That’s probably why the habit remains.