[Development] Exporting classes wholesale

[Marc Mutz]

Hi,

TL;DR: Never export non-polymorphic classes at class-level and always 
define destructors of polymorphic classes, and define them out-of-line.


The class-level export macro used to be a trigger for syncqt to generate 
a forwarding QClass header, so every public class had them in the past, 
and we have a rich history of exported classes.

Over the years, however, we have had a variety of issues with such 
classes as opposed to those that export only member functions that are 
defined out-of-line and expected to be called from outside the module.

An (incomplete) list of these issues can be found in TTLOFIR:
https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews#Value_Classes Item 6

These issues are subtle and many took _months_ to figure out.

We have therefore begun, over time, to favour _not_ exporting 
non-polymorphic classes at class level in order to limit similar 
problems going forward. Syncqt has been fixed ages ago. Yes, it's more 
work to repeat the macro on most function instead of once at class 
level, but we trade more obvious typing against less subtle issues 
cropping up later. Minimizing typing for Qt class authors is not a 
design goal (and QtC's ALT-Down functionality for block editing works 
wonders for adding this).

I've distilled this knowledge over time into the rule combination you 
see in the TL;DR: at the top of this email. This combination of rules 
has no False Positives that I am aware of. That means following these 
rules, your code is _always_ correct (within the scope of the rules).

It's pretty rare to find rules in C++ that are that clear-cut. It's 
usually "prefer to" or "avoid', with tons of exceptions.

Not here. There are no exceptions, and I think this will be one of the 
first Axivion checks we'd enable if and when we get to run it on the CI 
and report results in Gerrit.

I'm happy to discuss alternatives with anyone who thinks we can relax 
that rule to continue to add non-polymorphic classes with class-level 
exports, but I expect more than "I don't like it". I expect a concrete 
alternative rule with a FP rate not higher than the one posted here. 
Note that I, at least, never saw most of the issues listed in TTLOFIR 
coming before they hit us. So it's also very hard to argue that some 
classes "never will run into this". We just don't know what else will 
crop up, until it does.

So I maintain that the TL;DR: is the soundest engineering practice for 
exporting classes, and should be followed - without exception.

Thanks,
Marc

PS: if you wonder about the second part of the rule, that's 
https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews#Polymorphic_Classes 
Item 1

-- 
Marc Mutz <marc.mutz at qt.io> (he/his)
Principal Software Engineer

The Qt Company
Erich-Thilo-Str. 10 12489
Berlin, Germany
www.qt.io

Geschäftsführer: Mika Pälsi, Juha Varelius, Jouni Lintunen
Sitz der Gesellschaft: Berlin,
Registergericht: Amtsgericht Charlottenburg,
HRB 144331 B