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)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox19 --- fixed
b2g18 --- fixed

People

(Reporter: espindola, Assigned: espindola)

Details

Attachments

(1 file, 1 obsolete file)

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.
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+
> >  #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.
Attached patch patchSplinter Review
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 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+
https://hg.mozilla.org/mozilla-central/rev/6ed470203a3b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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+
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.
Whiteboard: [status-b2g18:fixed]
You need to log in before you can comment on or make changes to this bug.