805416 - refactor macros to avoid the need for empty macro arguments

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=b5224e8eba81

The alternatives are:
* Require a new c/c++ standard
* Ignore the warnings
* Drop -z,defs when linking on linux

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 675108 [details] [diff] [review]
patch

Review of attachment 675108 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/SHA1.h
@@ +43,5 @@
>      unsigned H[22]; /* 5 state variables, 16 tmp values, 1 extra */
>      bool mDone;
>  
>    public:
> +    MFBT_API_DIRECTIVE SHA1Sum();

I'm not convinced this is any better, especially considering how longer it is. Punting to Jeff.

@@ +49,5 @@
>      static const size_t HashSize = 20;
>      typedef uint8_t Hash[HashSize];
>  
>      /* Add len bytes of dataIn to the data sequence being hashed. */
> +    MFBT_API_DIRECTIVE void update(const void* dataIn, uint32_t len);

Why replace these?

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> > +    MFBT_API_DIRECTIVE SHA1Sum();
> 
> I'm not convinced this is any better, especially considering how longer it
> is. Punting to Jeff.

I can change the name if you want. If we are going to keep the -Wl,-z,defs, we probably need a way of avoiding the warnings.

> @@ +49,5 @@
> >      static const size_t HashSize = 20;
> >      typedef uint8_t Hash[HashSize];
> >  
> >      /* Add len bytes of dataIn to the data sequence being hashed. */
> > +    MFBT_API_DIRECTIVE void update(const void* dataIn, uint32_t len);
> 
> Why replace these?

Consistency. If we need MFBT_API_DIRECTIVE, we can do without MFBT_API and I was hopping to remove it in a followup patch (and then rename MFBT_API_DIRECTIVE to just MFBT_API).

Comment 4

[Mike Hommey [:glandium]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #3)
> I can change the name if you want. If we are going to keep the -Wl,-z,defs,
> we probably need a way of avoiding the warnings.

What warnings does using MFBT_ABI() add?

> > Why replace these?
> 
> Consistency. If we need MFBT_API_DIRECTIVE, we can do without MFBT_API and I
> was hopping to remove it in a followup patch (and then rename
> MFBT_API_DIRECTIVE to just MFBT_API).

Then why not replace MFBT_API entirely in this bug? It's not like nscore.h macros, it's used almost nowhere. That being said, I still don't see the benefit of avoiding MFBT_API().

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Mike Hommey [:glandium] from comment #4)
> (In reply to Rafael Ávila de Espíndola (:espindola) from comment #3)
> What warnings does using MFBT_ABI() add?

SHA1.h:47:14: warning: invoking macro MFBT_API argument 1: empty macro arguments are undefined in ISO C90 and ISO C++98

> Then why not replace MFBT_API entirely in this bug? It's not like nscore.h
> macros, it's used almost nowhere. That being said, I still don't see the
> benefit of avoiding MFBT_API().

I will give that a try.

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Change all uses of MFBT_API and MFBT_DATA

https://tbpl.mozilla.org/?tree=Try&rev=178439f07811

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Change all uses of MFBT_API and MFBT_DATA

Correct patch this time, sorry.
https://tbpl.mozilla.org/?tree=Try&rev=96729f9b6e7e

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 675165 [details] [diff] [review]
Change all uses of MFBT_API and MFBT_DATA

Review of attachment 675165 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, MFBT_API_DIRECTIVE is excessively long, we shouldn't make users use that.

The double-conversion changes need to be added to mfbt/double-conversion/update.sh.  Which means the existing MFBT API patch there needs to be removed at the same time, too.  It's probably a good idea to document which revision of double-conversion we've imported, in update.sh, as well -- I can't find it anywhere in the tree, presumably only in Bugzilla archaeology.  r- for this bit, but the rest is basically fine.

Somewhere in Types.h you should mention that the *_DIRECTIVE macros shouldn't be directly used, I think.  Not sure about a good place.

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.

::: mfbt/HashFunctions.cpp
@@ +11,5 @@
>  #include <string.h>
>  
>  namespace mozilla {
>  
> +MFBT_API uint32_t

Is it necessary to tag this at all, or will this be inferred given the HashBytes symbol already includes it?  Just seems nice to not have noise if we can avoid it (I don't know if we can).

::: mfbt/Types.h
@@ +68,5 @@
>   * depending upon the compilation mode.
>   */
>  #ifdef _WIN32
>  #  if defined(__MWERKS__)
> +#    define MOZ_IMPORT_API_DIRECTIVE

/* nothing */

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: patch on top of the double-conversion update

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b511cc69f717

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b511cc69f717

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 676325 [details] [diff] [review]
patch on top of the double-conversion update

[Triage Comment]
a-b2g18=me; we need this for bug 804303.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a04be19aa439