Open Bug 456388 Opened 16 years ago Updated 2 years ago

Remove PR_STATIC_CALLBACK and PR_CALLBACK(_DECL) from the tree

Categories

(Core :: General, defect)

defect

Tracking

()

mozilla1.9.1b2

People

(Reporter: Swatinem, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

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.
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.
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.
Assignee: nobody → arpad.borsos
Status: NEW → ASSIGNED
Attachment #340739 - Flags: superreview?(wtc)
Attachment #340739 - Flags: review?(wtc)
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.
Attachment #340741 - Flags: superreview?(brendan)
Attachment #340741 - Flags: review?(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
Attachment #340741 - Flags: superreview?(brendan)
Attachment #340741 - Flags: superreview+
Attachment #340741 - Flags: review?(brendan)
Attachment #340741 - Flags: review+
Keywords: checkin-needed
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.
Attachment #340741 - Attachment is obsolete: true
Attachment #342268 - Flags: superreview+
Attachment #342268 - Flags: review+
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.
Attachment #340739 - Flags: superreview?(wtc)
Attachment #340739 - Flags: review?(wtc)
Attachment #340739 - Flags: review-
(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 on attachment 342268 [details] [diff] [review]
unbitrot for mozilla-central
[Checkin: Comment 8]

http://hg.mozilla.org/mozilla-central/rev/eee86e5513ad
Attachment #342268 - Attachment description: unbitrot for mozilla-central → unbitrot for mozilla-central [Checkin: Comment 8]
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite-
Keywords: checkin-needed
OS: Linux → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch remove macro usage from nss (obsolete) — Splinter Review
Patch cleans up the macros from nss.
Attachment #343868 - Flags: review?(wtc)
A followup patch that cleans up all the remaining macros from mozilla-central
Attachment #343869 - Flags: superreview?(brendan)
Attachment #343869 - Flags: review?(brendan)
Comment on attachment 343869 [details] [diff] [review]
followup for mozilla-central [Checkin: Comment 13]

r/sr=me.

/be
Attachment #343869 - Flags: superreview?(brendan)
Attachment #343869 - Flags: superreview+
Attachment #343869 - Flags: review?(brendan)
Attachment #343869 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Attachment #343869 - Attachment description: followup for mozilla-central → followup for mozilla-central [Checkin: Comment 13]
Dan: I hope you are the right peer for such a comm-central wide (but mostly mailnews) patch.
Attachment #344138 - Flags: superreview?(dmose)
Attachment #344138 - Flags: review?(dmose)
Comment on attachment 344138 [details] [diff] [review]
remove macro usage from comm-central

Patch much appreciated; thanks Arpad!
Attachment #344138 - Flags: superreview?(dmose)
Attachment #344138 - Flags: superreview+
Attachment #344138 - Flags: review?(dmose)
Attachment #344138 - Flags: review+
Comment on attachment 344138 [details] [diff] [review]
remove macro usage from comm-central

"abort: patch failed to apply"
(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.
Status: REOPENED → ASSIGNED
Keywords: checkin-needed
Updated to tip, r/sr+ as per comment 15.
Attachment #344138 - Attachment is obsolete: true
Attachment #345066 - Flags: superreview+
Attachment #345066 - Flags: review+
Comment on attachment 345066 [details] [diff] [review]
cleanup, comm-central, updated to tip
[Checkin: Comment 19]

http://hg.mozilla.org/comm-central/rev/ea049a1644bd
Attachment #345066 - Attachment description: cleanup, comm-central, updated to tip → cleanup, comm-central, updated to tip [Checkin: Comment 19]
Attachment #343868 - Attachment is obsolete: true
Attachment #343868 - Flags: review?(wtc)
Assignee: arpad.borsos → nobody
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.