Closed Bug 805416 Opened 7 years ago Closed 7 years ago

refactor macros to avoid the need for empty macro arguments

Categories

(Core :: MFBT, defect)

x86_64
Linux
defect
Not set

Tracking

()

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

People

(Reporter: espindola, Assigned: espindola)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch patch (obsolete) — Splinter Review
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
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #675108 - Flags: review?(mh+mozilla)
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?
Attachment #675108 - Flags: review?(mh+mozilla) → review?(jwalden+bmo)
> > +    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).
(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().
(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.
Correct patch this time, sorry.
https://tbpl.mozilla.org/?tree=Try&rev=96729f9b6e7e
Attachment #675162 - Attachment is obsolete: true
Attachment #675162 - Flags: review?(jwalden+bmo)
Attachment #675162 - Flags: feedback?(mh+mozilla)
Attachment #675165 - Flags: review?(jwalden+bmo)
Attachment #675165 - Flags: feedback?(mh+mozilla)
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 */
Attachment #675165 - Flags: review?(jwalden+bmo) → review-
Attachment #675108 - Flags: review?(jwalden+bmo)
Attachment #675108 - Attachment is obsolete: true
Attachment #675165 - Attachment is obsolete: true
Attachment #675165 - Flags: feedback?(mh+mozilla)
Attachment #676325 - Flags: review?(jwalden+bmo)
Attachment #676325 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/b511cc69f717
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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.
Attachment #676325 - Flags: approval-mozilla-b2g18+
Whiteboard: [status-b2g18:fixed]
You need to log in before you can comment on or make changes to this bug.