456388 - Remove PR_STATIC_CALLBACK and PR_CALLBACK(_DECL) from the tree

Status: NEW

(Core :: General, defect)

Description

[Arpad Borsos [:Swatinem]]

Similar to bug 398946. Once bug 433790 is done and WIN16 is removed, all architectures define those macros the same so they can be removed.
PR_CALLBACK = nothing
PR_CALLBACK_DECL = nothing
PR_STATIC_CALLBACK() = static

Core Gecko has dropped support for WIN16 long ago so it could take a headstart here and clean those macros up before NSPR.

Comment 1

[Wan-Teh Chang]

We need to keep the trivial definitions of these macros around
for backward compatibility.  Mozilla is not the only user of
NSPR, so there may be non-Mozilla code out there that uses these
macros.

Comment 2

[Arpad Borsos [:Swatinem]]

Attachment: remove macro usage from nspr

This patch removes nspr internal usage of those macros, they are however still defined in prtypes.h. The patch is based on top of bug #433790.

Comment 3

[Arpad Borsos [:Swatinem]]

Attachment: remove macro usage from mozilla-central

This patch removes the macros from the complete mozilla-central tree (minus nsprpub and security). I hope brendan is the right peer for this tree-wide patch.

Comment 4

[Brendan Eich [:brendan]]

Patch looks fine at a glance, main thing is warning and error free (net of existing warnings) compilation. Be great if we could be sure statically, but probably not worth it. Cc'ing static analysis folks just in case this is of interest.

/be

Comment 5

[Arpad Borsos [:Swatinem]]

Attachment: unbitrot for mozilla-central [Checkin: Comment 8]

Updated to work on top of nsDocument changes made by bug 433616. r/sr+ as per comment 4. Bug 433616 added some more static callbacks but I will do them in a followup patch.

Comment 6

[Wan-Teh Chang]

Comment on attachment 340739 [details] [diff] [review]
remove macro usage from nspr

Thanks for the patch.

We need to keep the definitions of these macros in prtypes.h for
backward compatibility.  There are many NSPR users outside the
Mozilla source tree.

I think we don't need to remove the use of these macros from
old code.  NSPR is now in a stable state.  I often read diffs
between NSPR releases to track down a regression.  So any noise
in the diffs makes the maintenance of NSPR difficult.

Comment 7

[Arpad Borsos [:Swatinem]]

(In reply to comment #6)
> (From update of attachment 340739 [details] [diff] [review])
> We need to keep the definitions of these macros in prtypes.h for
> backward compatibility.  There are many NSPR users outside the
> Mozilla source tree.

The patch still keeps the definitions. It just moves the defines from the platform specific ifdefs into a non platform specific part.

I also understand your concerns about maintaining the code. However my personal opinion is that less (confusing) macros are helping the learning curve of potential new contributors.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 342268 [details] [diff] [review]
unbitrot for mozilla-central
[Checkin: Comment 8]

http://hg.mozilla.org/mozilla-central/rev/eee86e5513ad

Comment 9

[Arpad Borsos [:Swatinem]]

Thanks for the checkin Serge.
However, this is not yet fixed. The macros are still used inside NSS (security/) and probably in comm-central. I haven't checked in comm-central yet.
And I will also do a followup patch for anything I missed inside m-c.
Is there any chance such a patch could get into NSS at all or should I rather save the time for something else?

Comment 10

[Arpad Borsos [:Swatinem]]

Attachment: remove macro usage from nss

Patch cleans up the macros from nss.

Comment 11

[Arpad Borsos [:Swatinem]]

Attachment: followup for mozilla-central [Checkin: Comment 13]

A followup patch that cleans up all the remaining macros from mozilla-central

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 343869 [details] [diff] [review]
followup for mozilla-central [Checkin: Comment 13]

r/sr=me.

/be

Comment 13

[Reed Loden [:reed]]

Comment on attachment 343869 [details] [diff] [review]
followup for mozilla-central [Checkin: Comment 13]

http://hg.mozilla.org/mozilla-central/rev/c53251d62ddf

Comment 14

[Arpad Borsos [:Swatinem]]

Attachment: remove macro usage from comm-central

Dan: I hope you are the right peer for such a comm-central wide (but mostly mailnews) patch.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 344138 [details] [diff] [review]
remove macro usage from comm-central

Patch much appreciated; thanks Arpad!

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 344138 [details] [diff] [review]
remove macro usage from comm-central

"abort: patch failed to apply"

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> However, this is not yet fixed.

In such cases, you can use the whiteboard to tell which patch to check in and/or to leave the bug open, etc.

Comment 18

[Arpad Borsos [:Swatinem]]

Attachment: cleanup, comm-central, updated to tip [Checkin: Comment 19]

Updated to tip, r/sr+ as per comment 15.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 345066 [details] [diff] [review]
cleanup, comm-central, updated to tip
[Checkin: Comment 19]

http://hg.mozilla.org/comm-central/rev/ea049a1644bd

Comment 20

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.