Closed
Bug 807112
Opened 12 years ago
Closed 12 years ago
change MOZ_EXPORT_API and MOZ_IMPORT_API to not take the type
Categories
(Core :: MFBT, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: espindola, Assigned: espindola)
Details
Attachments
(1 file, 1 obsolete file)
18.82 KB,
patch
|
Waldo
:
review+
justin.lebar+bug
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
form Waldo in bug 805416: For consistency, and (arguably) the least obfuscation, we should consider making MOZ_EXPORT_API and MOZ_IMPORT_API not take the type, and just have them function like MFBT_API does after this patch. Separate bug and patch, definitely.
Assignee | ||
Comment 1•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=051c059bad17
Attachment #677046 -
Flags: review?(jwalden+bmo)
Comment 2•12 years ago
|
||
Comment on attachment 677046 [details] [diff] [review] change MOZ_EXPORT_API and MOZ_IMPORT_API to not take the type Review of attachment 677046 [details] [diff] [review]: ----------------------------------------------------------------- ::: memory/mozjemalloc/jemalloc.h @@ +58,5 @@ > #if defined(MOZ_MEMORY_LINUX) || defined(MOZ_MEMORY_BSD) > __attribute__((weak)) > #endif > #if defined(MOZ_JEMALLOC) > +MOZ_IMPORT_API int wrap(nallocm)(size_t *rsize, size_t size, int flags); Is it possible that MOZ_IMPORT_API is redundant with the __attribute__((weak)) above here? The BSD bit says maybe not, maybe, but I figure I should ask just in case. Random un-descriptively-encapsulated __attribute__(())s are best avoided. ::: mfbt/Types.h @@ +38,4 @@ > * > * api.c: > + * MOZ_EXPORT_API int MeaningOfLife(void) { return 42; } > + * MOZ_EXPORT_DATA int LuggageCombination = 12345; Hmm. These macros on the definitions aren't actually necessary, are they, because they'll get applied by the initial definition. So we might as well remove the api.c part and the api.h heading, then. Right? @@ +55,5 @@ > # else > # define MOZ_EXTERNAL_VIS > # endif > +# define MOZ_EXPORT_API MOZ_EXTERNAL_VIS > +# define MOZ_EXPORT_DATA MOZ_EXTERNAL_VIS Let's have the ifdefs define MOZ_EXPORT_API, then have a #define MOZ_EXPORT_DATA MOZ_EXPORT_API. But, hm. If MOZ_EXPORT_API is always the same as MOZ_EXPORT_DATA, why even have this distinction at all? Why not just MOZ_EXPORT?
Attachment #677046 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 3•12 years ago
|
||
> > #if defined(MOZ_MEMORY_LINUX) || defined(MOZ_MEMORY_BSD) > > __attribute__((weak)) > > #endif > > #if defined(MOZ_JEMALLOC) > > +MOZ_IMPORT_API int wrap(nallocm)(size_t *rsize, size_t size, int flags); > > Is it possible that MOZ_IMPORT_API is redundant with the > __attribute__((weak)) above here? The BSD bit says maybe not, maybe, but I > figure I should ask just in case. Random un-descriptively-encapsulated > __attribute__(())s are best avoided. My guess is that this is the same hack we do in Types.h and should be using MOZ_GLUE_IN_PROGRAM, but it is hard to be sure. > @@ +55,5 @@ > > # else > > # define MOZ_EXTERNAL_VIS > > # endif > > +# define MOZ_EXPORT_API MOZ_EXTERNAL_VIS > > +# define MOZ_EXPORT_DATA MOZ_EXTERNAL_VIS > > Let's have the ifdefs define MOZ_EXPORT_API, then have a #define > MOZ_EXPORT_DATA MOZ_EXPORT_API. > > But, hm. If MOZ_EXPORT_API is always the same as MOZ_EXPORT_DATA, why even > have this distinction at all? Why not just MOZ_EXPORT? Will do that.
Assignee | ||
Comment 4•12 years ago
|
||
Would you mind a final look? There were some fairly large changes. https://tbpl.mozilla.org/?tree=Try&rev=7f6646e265e8
Attachment #677046 -
Attachment is obsolete: true
Attachment #677110 -
Flags: review?(jwalden+bmo)
Comment 5•12 years ago
|
||
Comment on attachment 677110 [details] [diff] [review] patch Review of attachment 677110 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Types.h @@ +26,5 @@ > > /* Implement compiler and linker macros needed for APIs. */ > > /* > + * MOZ_EXPORT is used to declare and define a method which is externally s/method/symbol/, or method or data, or something? It ain't just for methods. And I guess it's for classes, too, so, um. I think symbol encapsulates all of them, but you're more familiar with the terminology for all this, maybe you have a better idea. @@ +52,5 @@ > # define MOZ_EXTERNAL_VIS __global > # else > # define MOZ_EXTERNAL_VIS > # endif > +# define MOZ_EXPORT MOZ_EXTERNAL_VIS Well, what I eventually came around to was this: # ifdef HAVE_VISIBILITY_ATTRIBUTE # define MOZ_EXPORT __attribute__((visibility("default"))) # elif defined(__SUNPRO_C) || defined(__SUNPRO_CC) # define MOZ_EXPORT __global # else # define MOZ_EXPORT /* nothing */ # endif i.e. with the one-shot MOZ_EXTERNAL_VIS macro gone.
Attachment #677110 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ed470203a3b
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6ed470203a3b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 8•11 years ago
|
||
Comment on attachment 677110 [details] [diff] [review] patch [Triage Comment] a-b2g18=me; we need this for bug 804303.
Attachment #677110 -
Flags: approval-mozilla-b2g18+
Comment 9•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/078191be0596
status-firefox19:
--- → fixed
Whiteboard: [status-b2g18:fixed]
Comment 10•11 years ago
|
||
Well, that was entertaining. OS.File had other instances of MOZ_EXPORT_API that needed changing, and I apparently had to do things the hard way to make it so. https://hg.mozilla.org/releases/mozilla-b2g18/rev/0782df39e525 https://hg.mozilla.org/releases/mozilla-b2g18/rev/bf602396ce66 https://hg.mozilla.org/releases/mozilla-b2g18/rev/cfe2094c0312 Fail.
status-b2g18:
--- → fixed
Whiteboard: [status-b2g18:fixed]
You need to log in
before you can comment on or make changes to this bug.
Description
•