GNOME Bugzilla – Bug 728031
Use visibility attributes/__declspec (dllexport) to export public symbols
Last modified: 2014-04-25 00:41:58 UTC
Hi, Following a conversation in e-mail with Piñeiro, it seems that it would be a good idea to make ATK like GLib and GTK+, to export the APIs using a visibility-based approach (for GCC builds) or a __declspec (dllexport) based approach. This is my attempt to do this part.
Created attachment 274078 [details] [review] Add atk/atkmacros.h Hi, This adds a public header to be included by all the other public headers of ATK so that it can be used to annotate all the public functions in the headers that can be defined in config.h to export the public symbols with compiler-specific directives.
Created attachment 274082 [details] [review] Update the Public Headers to include atk/atkmacros.h and Annotate the public symbols Hi, This updates the public headers to include atk/atkmacros.h, and annotates the public symbols accordingly with ATK_AVAILABLE_IN_ALL. The G_DEPRECATED* items are also updated to ATK_DEPRECATED*, as in atk/atkmacros.h, so that the _ATK_EXTERN can be used on all the symbols, where it can be defined during build-time to compiler directives to export the symbols. p.s. for atk/atkmacros.h, I did not use things like ATK_AVAILABLE_IN* as I am not sure how this should be defined there.
Created attachment 274083 [details] [review] Update the ATK c-sources to all include config.h Hi, This updates all the ATK c-sources to include config.h before anything else. This makes sure that we use the build-time definition of _ATK_EXTERN.
Created attachment 274085 [details] [review] Update the generation of the enumeration sources Hi, This updates the generation of the enumeration sources so that atk/atkmacros.h is included, the symbols are decorated with ATK_AVAILABLE_IN_ALL, and that config.h is included in the generated enumeration c-source. I will go back to the configure.ac regarding the compiler directive to be used for _ATK_EXTERN later, and will update config.h.win32.in after this is deemed ok. With blessings, thank you!
(In reply to comment #0) > Hi, Hi, thanks for the patches. Today I will not have time to review them, will try next week, but I have one question. > Following a conversation in e-mail with Piñeiro, it seems that it would be a > good idea to make ATK like GLib and GTK+, to export the APIs using a > visibility-based approach (for GCC builds) or a __declspec (dllexport) based > approach. This is my attempt to do this part. Due that conversation I created bug 720203. So just to confirm, is that bug and this one the same? If that so, feel free to close 720203 as duplicate (as current one is the one with the patches).
*** Bug 720203 has been marked as a duplicate of this bug. ***
Created attachment 274225 [details] [review] Add atk/atkmacros.h (take ii) Hi, This updates the original version of the patch so that the inclusion guard macros are more consistent with the other header files.
Created attachment 274226 [details] [review] Annotate the public headers to decorate the public symbols for export Hi, As the original patch (#274082) tried to include atk/atkmacros.h in all the public headers, simplify a bit by having atk/atkmacros.h included before any other headers in atk/atkobject.h, which will be included by most of the public headers, and include atk/atkmacros.h only when atk/atkobject.h is not included in the headers.
Created attachment 274227 [details] [review] Annotate the public headers to decorate the public symbols for export (take ii) Hi, Sorry, I forgot that I needed to update the definitions for ATK_VAR so that public variables/constants can be exported as well.
Created attachment 274228 [details] [review] Update the autotools files for using visibility to export the public symbols Hi, The last patch for this set is to update the autotools files to export the public symbols using visibility attributes during the build. With blessings, thank you!
Review of attachment 274083 [details] [review]: Looks ok to me.
Review of attachment 274085 [details] [review]: Looks good to me.
Review of attachment 274225 [details] [review]: Looks good, just a minor issue. But feel free to commit it by fixing it. ::: atk/atkmacros.h @@ +1,2 @@ +/* ATK - Accessibility Toolkit + * Copyright 2001 Sun Microsystems Inc. We are not on 2001 anymore ;) And AFAIK Sun Microsystems is not around anymore. So please update the date and the copyright assignment (to your employer if this was related to working with it, or directly to you if was made on personal time)
Review of attachment 274225 [details] [review]: Second review. Sorry I realized something when I started to review the following patch. ::: atk/atkmacros.h @@ +43,3 @@ +#endif + +#define ATK_AVAILABLE_IN_ALL _ATK_EXTERN So only an ATK_AVAILABLE_IN_ALL is defined, so that means that currently all the public methods will be marked as available since the beginning of time. And Im not really comfortable with exposing that. I made some effort to improve the documentation in order to list the added methods for each major release. See following link at the end: https://developer.gnome.org/atk/2.12/ Would it be possible to add some ATK_VERSION_AVAILABLE_IN_X_Y macros (like those available for gtk and clutter)? Anyway, it is true that doing it for all the symbols would be a big job. So what about refining for the symbols added after the 2.0 release? After all 2.0 is API compatible with 1.0. We can consider that any method coming from 1.0 is available for any 2.X releases. That would mean that you would only need mark the following methods: https://developer.gnome.org/atk/2.12/api-index-2-2.html https://developer.gnome.org/atk/2.12/api-index-2-8.html https://developer.gnome.org/atk/2.12/api-index-2-10.html https://developer.gnome.org/atk/2.12/api-index-2-12.html Being 2.12 the one that added more methods.
Review of attachment 274227 [details] [review]: Looks good in general. The only problem is that every public method is marked as ATK_AVAILABLE_IN_ALL, when some of those public methods were published on recent releases (ie 2.12). See the review of "Add atk/atkmacros.h (take ii)" patch for more info about it.
Review of attachment 274228 [details] [review]: Looks good to me. And having the atk.symbols removed is a good step forward, as it was a source of several mistakes.
Created attachment 275046 [details] [review] Add Version/Deprecation Macros in atk/atkversion.h.in Hi, Coming to think of it, as it is desirable to add ATK_AVAILABLE_IN_X_Y and so on for the new symbol exporting mechanism, probably it is better to add these in atk/atkversion.h.in, and make sure later that it is included in some way by the header files, as we also needed to check against the version of ATK that is being built/linked to. So, this is my take on this...
Created attachment 275047 [details] [review] atk/Makefile.am: Update Generation of Enumeration Sources (take ii) Hi, This updates on the previous patch for the same purpose, as we need to include atk/atkversion.h instead of atk/atkmacros.h.
Created attachment 275048 [details] [review] Annotate the public headers to decorate the public symbols for export (take iii) Hi, According to Piñeiro's suggestions, this annotates the public APIs according to the 2.x stable release series (the ones introduced/deprecaed at or before atk-2.0.0 is still marked as ATK_AVAILABLE_IN_ALL/ATK_DEPRECATED (or ATK_DEPRECATED_FOR). The _get_type() APIs in the public headers, if the header was introduced after 2.0.0, is also marked with the macro for the stable 2.x release series, for consistency reasons. Oh, I forgot to mention, in my patch for atk/atkversion.h.in, I added macros for 2.4 and 2.6, even when there were no new APIs, to maintain consistency (as this is done for GLib/GTK+/Clutter). Please let me know if these are preferred to be not included. So, these are my take for now. With blessings, thank you!
Review of attachment 275046 [details] [review]: Looks good to me.
Review of attachment 275047 [details] [review]: Looks good to me.
Review of attachment 275048 [details] [review]: Looks good to me. Thanks for all those patches!
Review of attachment 275046 [details] [review]: Hi, This patch was committed as 122236e1, ...
Review of attachment 275048 [details] [review]: ... this patch was committed as ca5ea1bd, ...
Review of attachment 274083 [details] [review]: ... this patch as d6ecbbbe, ...
Review of attachment 275047 [details] [review]: ... this patch as 9a1ae853, ...
Review of attachment 274228 [details] [review]: ... and last but not least, this patch was commited as 41442d82. Thanks for the reviews, I will close this bug shortly. With blessings, thank you!