Do not leak symbols of pimpl classes, protect with Q_DECL_HIDDEN
ClosedPublic
Actions

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

Details

Reviewers

cfeck
dfaure
vkrause
mlaurent

Commits

R236:359c0d18fecb: Do not leak symbols of pimpl classes, protect with Q_DECL_HIDDEN

Summary

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

Repository

R236 KWidgetsAddons

Branch

dontleaksymbolsofpimplclasses

Lint

No Linters Available

Unit

No Unit Test Coverage

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:
https://mail.kde.org/pipermail/kde-frameworks-devel/2015-August/025958.html
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 added a reviewer: mlaurent.Aug 12 2017, 8:46 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

mpyne mentioned this in D7281: KCoreAddons: Enforce hidden symbol visibility in nested private classes.Aug 12 2017, 11:41 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.

[1] https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

Closed by commit R236:359c0d18fecb: Do not leak symbols of pimpl classes, protect with Q_DECL_HIDDEN (authored by kossebau). · Explain WhyAug 15 2017, 1:14 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			src/kassistantdialog.cpp (2 lines)
M			src/kcapacitybar.cpp (2 lines)
M			src/kcharselect.cpp (2 lines)
M			src/kcolorbutton.cpp (2 lines)
M			src/kdatepicker.cpp (2 lines)
M			src/kfontchooser.cpp (2 lines)
M			src/kfontrequester.cpp (2 lines)
M			src/kfontsizeaction.cpp (2 lines)
M			src/kled.cpp (2 lines)
M			src/kmimetypechooser.cpp (2 lines)
M			src/knewpassworddialog.cpp (2 lines)
M			src/knewpasswordwidget.cpp (2 lines)
M			src/kpagewidgetmodel.cpp (2 lines)
M			src/kpassworddialog.cpp (2 lines)
M			src/kpixmapregionselectordialog.cpp (2 lines)
M			src/kpixmapregionselectorwidget.cpp (2 lines)
M			src/kpixmapsequence.cpp (2 lines)
M			src/kpixmapsequenceoverlaypainter.cpp (2 lines)
M			src/kpixmapsequencewidget.cpp (2 lines)
M			src/kpopupframe.cpp (2 lines)
M			src/kratingpainter.cpp (2 lines)
M			src/kratingwidget.cpp (2 lines)
M			src/kselector.cpp (2 lines)
M			src/ksplittercollapserbutton.cpp (2 lines)
M			src/ktitlewidget.cpp (2 lines)
M			src/ktoggleaction.cpp (2 lines)
M			src/ktogglefullscreenaction.cpp (2 lines)
M			src/ktoolbarlabelaction.cpp (2 lines)
M			src/ktoolbarpopupaction.cpp (2 lines)
M			src/ktooltipwidget.cpp (2 lines)
M			src/kurllabel.cpp (2 lines)
M			src/kxyselector.cpp (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	18062	ddc289b		Aug 12 2017, 7:25 PM	★	★
Diff 2	18163	1849447	R236:359c0d18fecbd45b77f27a5ca1fda5533f5ac73a	Aug 15 2017, 1:14 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
c27a1213a45d	4ad92a00d6ce	ddc289b43c9c	Friedrich W. H. Kossebau	Do not leak symbols of pimpl classes, protect with Q_DECL_HIDDEN (Show More…)	Aug 12 2017, 7:23 PM

Do not leak symbols of pimpl classes, protect with Q_DECL_HIDDENClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 18062

src/kassistantdialog.cpp

src/kcapacitybar.cpp

src/kcharselect.cpp

src/kcolorbutton.cpp

src/kdatepicker.cpp

src/kfontchooser.cpp

src/kfontrequester.cpp

src/kfontsizeaction.cpp

src/kled.cpp

src/kmimetypechooser.cpp

src/knewpassworddialog.cpp

src/knewpasswordwidget.cpp

src/kpagewidgetmodel.cpp

src/kpassworddialog.cpp

src/kpixmapregionselectordialog.cpp

src/kpixmapregionselectorwidget.cpp

src/kpixmapsequence.cpp

src/kpixmapsequenceoverlaypainter.cpp

src/kpixmapsequencewidget.cpp

src/kpopupframe.cpp

src/kratingpainter.cpp

src/kratingwidget.cpp

src/kselector.cpp

src/ksplittercollapserbutton.cpp

src/ktitlewidget.cpp

src/ktoggleaction.cpp

src/ktogglefullscreenaction.cpp

src/ktoolbarlabelaction.cpp

src/ktoolbarpopupaction.cpp

src/ktooltipwidget.cpp

src/kurllabel.cpp

src/kxyselector.cpp

Do not leak symbols of pimpl classes, protect with Q_DECL_HIDDEN
ClosedPublic
Actions

Revision Contents
Changeset List