Do not leak symbols of pimpl classes, protect with Q_DECL_HIDDEN

Authored by kossebau on Aug 12 2017, 7:25 PM.



Nested classes inherit the visibility of their outer class,
so all the KClass::Private symbols have also been exported
if the ones of KClass were.

Best fix would be to change all pimpl classes to be non-nested,
but that needs a bigger patch as some nested classes directly
access types and properties of their outer classes.
Left to do for someone else :)

Diff Detail

R236 KWidgetsAddons
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Aug 12 2017, 7:25 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 12 2017, 7:25 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript

@dfaure @vkrause I was surprised to see those symbols still exported. From what I remember in this ml thread in 2015 I would have assumed you dealt with that already:
After all that had inspired me to do similar fixes across a few projects :)

So was there a showstopper perhaps? I would have assumed that those symbols of the private classes could be considered just leaked and thus removable, without breaking any official ABI.

For effects, I have not measured any linking times or symbol number, due to no experience how to do that properly. Just can tell that in debug builds the stripped size shrinks from 1805464 to 1792528 bytes for me, so by 0.77 %. Not impressive, but small things add up, and this one was easy to do :)

dfaure accepted this revision.Aug 12 2017, 8:49 PM

Urgh I completely forgot about this after writing the perl script... Indeed that's much simpler than renaming the private classes.

This revision is now accepted and ready to land.Aug 12 2017, 8:49 PM

Okay, if just forgotten then I have patches for all of KF5, would just go ahead and add Q_DECL_HIDDEN whereever it seems useful.
Would do so upcoming Monday.

apol added a subscriber: apol.Aug 13 2017, 5:35 PM
This comment was removed by apol.

As a side note: Leaking the visibility of nested classes is almost something that could be mentioned in [1]. Then again, this is only a BC issue, if these nested classes are used/accessible in public API.


This revision was automatically updated to reflect the committed changes.