305278 - plevent.h gives bad advice

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The comment in plevent.h gives bad advice in two ways:
 * it recommends using casts when none should be needed, and the habit of using
them can cover up real bugs
 * it omits PR_CALLBACK in the example handlers, which would break any build
that defines it to something interesting (e.g., the OS/2 VisualAge build)

I'm more concerned about the first, really.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 2

[Doug Turner (:dougt)]

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

the change to the example is that you are now droping the |static| keyword
(that is what PR_STATIC_CALLBACK gave you).

Do you want to run through the tree and try to remove the casts?

Comment 3

[Mike Kaply [:mkaply]]

Actually, I'm more concerned that the example never had the static keyword. 

I'll remove some of the casts and see what it does to the OS/2 build.

Note we don't build VisualAge Mozilla anymore - we are GCC only.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Having static is nice, but not important -- it just makes the function have
internal linkage, so the name isn't exposed to anything outside the file in
which it lives.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Checked in to trunk.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to correct callers (and re-correct comment)

So I missed one thing in the comment -- the argument type.  I still think it's far better this way.  Note the number of callers who passed a function returning void in place of one returning void* (not harmful in this case, but a very bad habit).

Comment 7

[Darin Fisher]

Comment on attachment 202839 [details] [diff] [review]
patch to correct callers (and re-correct comment)

sr=darin

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk.