Bug 23275 - Add C++ visibility support
Summary: Add C++ visibility support
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-qt (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/an...
Whiteboard:
Keywords: patch
Depends on:
Blocks: 22910
  Show dependency treegraph
 
Reported: 2009-08-12 09:25 UTC by Andre Moreira Magalhaes
Modified: 2009-10-06 14:00 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Exported symbols (180.22 KB, text/plain)
2009-08-13 17:05 UTC, Andre Moreira Magalhaes
Details
Exported symbols (166.34 KB, text/plain)
2009-08-18 15:58 UTC, Andre Moreira Magalhaes
Details

Description Andre Moreira Magalhaes 2009-08-12 09:25:09 UTC
Add C++ visibility support
Comment 1 Simon McVittie 2009-08-13 10:29:42 UTC
Looks good in principle, but before we merge this, could you please hack up the build system so it temporarily produces a shared library, pass it through nm and c++filt, attach the result, and check that there aren't symbols that appear in our ABI but that we are not prepared to commit to?

A command like this might be helpful:

nm .libs/libtelepathy-qt4.a | grep " [DT] " | c++filt | sort -k3
Comment 2 Andre Moreira Magalhaes 2009-08-13 17:05:24 UTC
Created attachment 28610 [details]
Exported symbols
Comment 3 Andre Moreira Magalhaes 2009-08-13 17:05:49 UTC
(In reply to comment #1)
> Looks good in principle, but before we merge this, could you please hack up the
> build system so it temporarily produces a shared library, pass it through nm
> and c++filt, attach the result, and check that there aren't symbols that appear
> in our ABI but that we are not prepared to commit to?
> 
> A command like this might be helpful:
> 
> nm .libs/libtelepathy-qt4.a | grep " [DT] " | c++filt | sort -k3
> 

Despite the XXX::Private methods I don't see any other symbol that should not be there. Should we add TELEPATHY_QT4_NO_EXPORT to all Private structs? Qt does not do it, as they use shared d pointers in some places, but we don't, at least for now. I would let it as is.

Also exported non-class methods, enableDebug, registerTypes, ...
Comment 4 Simon McVittie 2009-08-18 08:12:33 UTC
Rejected for now, please re-add patch tag when you've fixed it.

(In reply to comment #3)
> Despite the XXX::Private methods I don't see any other symbol that should not
> be there. Should we add TELEPATHY_QT4_NO_EXPORT to all Private structs?

Yes, definitely. At the moment, they're part of our shared library ABI; that means that we should never remove, rename, or change the semantics of these methods, which isn't desirable.

Also, if we have any private methods on public objects, we should make sure they're not exported either.

> Also exported non-class methods, enableDebug, registerTypes, ...

The extra patch looks fine.
Comment 5 Andre Moreira Magalhaes 2009-08-18 15:57:44 UTC
(In reply to comment #4)
> Rejected for now, please re-add patch tag when you've fixed it.
> 
> (In reply to comment #3)
> > Despite the XXX::Private methods I don't see any other symbol that should not
> > be there. Should we add TELEPATHY_QT4_NO_EXPORT to all Private structs?
> 
> Yes, definitely. At the moment, they're part of our shared library ABI; that
> means that we should never remove, rename, or change the semantics of these
> methods, which isn't desirable.
> 
> Also, if we have any private methods on public objects, we should make sure
> they're not exported either.
Done.
Comment 6 Andre Moreira Magalhaes 2009-08-18 15:58:36 UTC
Created attachment 28757 [details]
Exported symbols
Comment 7 Simon McVittie 2009-08-19 04:42:38 UTC
Comment on attachment 28757 [details]
Exported symbols

>000000000007e000 T Tp::Account::gotAvatar(QDBusPendingCallWatcher*)
>0000000000086450 T Tp::Account::gotMainProperties(QDBusPendingCallWatcher*)

That doesn't look right. Seems we need to unexport private methods explicitly...
Comment 8 Andre Moreira Magalhaes 2009-10-06 13:29:55 UTC
Updated to latest changes
Comment 9 Andre Moreira Magalhaes 2009-10-06 14:00:25 UTC
Fixed upstream, will be in next release 0.2


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.