33610 - PR_CALLBACK and PR_CALLBACK_PTR

Status: RESOLVED INVALID

(NSPR :: NSPR, defect, P3)

Description

[Mike Kaply [:mkaply]]

Chris Blizzard told me to go ahead and open this as a bug:


We have an interesting problem on the OS/2 platform.

Take the following function declaration:

static PR_CALLBACK PLHashNumber _hashValue(const void *key)

on all platforms but Win16, PR_CALLBACK is not defined. On Win16 it is
used for a linkage. For Win16, this linkage goes before the return type.

However, on OS/2, we have to specify the linkage between the return type
and the function like this:

static PLHashNumber PR_CALLBACK _hashValue(const void *key)

PR_CALLBACK is defined to _Optlink, our linkage.

To fix this problem, we must add #ifdefs to all functions that use
PR_CALLBACK.


On 4.X, a solution was created for this problem but apparently not
ported to Mozilla. The solution is to use a macro like this:

For Win16:

    #define PR_CALLBACK_PTR(__x)    PR_CALLBACK __x

For OS/2:

    #define PR_CALLBACK_PTR(__x)    __x PR_CALLBACK

For all other platforms:

    #define PR_CALLBACK_PTR(__x)    PR_CALLBACK __x


Then the above function becomes:

static PR_CALLBACK_PTR(PLHashNumber) _hashValue(const void *key)

and does not have to be #ifdeffed for any platforms.

We would like to suggest that this change be made to prtypes.h

Another option would be to define something like PR_LINKAGE like this:

static PR_CALLBACK PLHashNumber PR_LINKAGE _hashValue(const void *key)

Where PR_LINKAGE only gets set on OS/2.

In addition, we have found a number of areas throughtout Mozilla that are NSPR 
callbacks but lack the PR_CALLBACK convention. We assume this is because WIN16 
is not needed for Mozilla.

Comment 1

[srinivas]

For some reason, PR_CALLBACK was originally defined without the return type as a 
parameter; it should have been defined similar to PR_EXPORT, etc.

For static callback functions on OS2 (XP_OS2_VACPP), PR_STATIC_CALLBACK(x) is 
defined correctly in prtypes.h

#define PR_STATIC_CALLBACK(__x) static __x _Optlink

Reassigning to wtc.

Comment 2

[Wan-Teh Chang]

Michael, can you use the existing PR_STATIC_CALLBACK macro
and declare your hash function like this?
PR_STATIC_CALLBACK(PLHashNumber) _hashValue(const void *key);

Larry, on Win16, can you put PR_CALLBACK *after* the return
type?

Comment 3

[Mike Kaply [:mkaply]]

The code in question is:

http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsHashtable.cpp#31

Looking at this code in particular, it probably should be a PR_STATIC_CALLBACK 
given that it is a static function, but we have found other places that are not 
static.

Would you prefer changing stuff from PR_CALLBACK to PR_STATIC_CALLBACK or from 
PR_CALLBACK to PR_CALLBACK_PTR, given that we are going to have to make changes 
anyway.

And I guess the other issue is how to handle this right now as this is a rather 
large issue.

I am researching to find out how many NSPR specific changes there are. Maybe 
most of this should go in as simple bugs against components.

Should the above code be PR_STATIC_CALLBACK?

Comment 4

[larryh (gone)]

It would appear that the browser code using PR_CALLBACK is busted. Here's the 
scoop:

The syntax defined for PR_CALLBACK is:

<scope> <type> PR_CALLBACK <functionName>(<argList>)

This has always been the syntax for using PR_CALLBACK. Currently, PR_CALLBACK 
expands to nothing on all platforms except Win16. Recall that Win16 is no longer 
supported and nobody builds things for it anymore, so any inappropriate use of 
PR_CALLBACK would go unnoticed. So, of course it works on all supported 
platforms. ... But OS/2, which requires that PR_CALLBACK expand to something 
that OS/2 compilers need.

Solution: Change the code using PR_CALLBACK to usse the correct syntax.

Comment 5

[Mike Kaply [:mkaply]]

So a few questions then:

1. Doesn't Win16 still need to be built for NSPR? Don't they want to keep having 
WIN16 stuff?

2. Would WIN16 support PR_CALLBACK in the "correct" place?

3. Who would be reponsible for making these kind of changes?

Comment 6

[larryh (gone)]

1. NSPR has not built for Win16 in many moons; last I recall trying to build 
NSPR for Win16 (3Q 1998), it had quit building. There is no requirement for 
building a Win16 Mozilla client, nor commercial client.

2. Yes. Last I looked. The test cases in mozilla/nsprpub/pr/tests/*.c that use 
PR_CALLBACK, compiled and ran last I built and ran them.

3. Who? Probably those own the parts with the inappropriate use of PR_CALLBACK, 
or someone who wants it to build and work for OS/2.

Comment 7

[Mike Kaply [:mkaply]]

OK, so I am still confused here.

It seems like we have a couple different problems:

Improper use of static PR_CALLBACK when it should be STATIC_PR_CALLBACK

Improper location of PR_CALLBACK

PR_CALLBACK_PTR possibly should be used.

I guess what I am trying to figure out is what is the right thing to do. Given 
the other STATIC_PR_CALLBACK_PTR(xxx) macros, I would think that 
PR_CALLBACK_PTR(xxx) would be the right thing.

My hope is that we could get this fixed the right way in one try so if anyone 
else runs into this, we could fix it.

If someone tells me exactly what to do, I would like to fix it and I would like 
to have this bug open to put in the code for reviews and for checkin.

Thanks