49641 - C style cast rather than C++ cast.

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P3)

Description

[Bruce Mitchener]

Warning 749: "../../dist/include/xptinfo.h", line 236 # The cast from 'const 
XPTConstValue *' to 'nsXPTCMiniVariant *' is performed as a 'reinterpret_cast'. 
This operation is non-portable and potentially unsafe.
            {return (nsXPTCMiniVariant*) &value;}

Comment 1

[Bruce Mitchener]

another one:

Warning 749: "xpcconvert.cpp", line 974 # The cast from 'nsXPCWrappedJS *' to 
'nsIXPCException *' is performed as a 'reinterpret_cast'. This operation is 
non-portable and potentially unsafe.
                        return (nsIXPCException*)
                               ^^^^^^^^^^^^^^^^^^

Comment 2

[Decklin Foster]

can we merge this with bug 49640?

Comment 3

[Bruce Mitchener]

I'd rather not. I've got more coming and my experience with having one bug with 
a lot of owners has sucked in the past.  I'd rather file a separate bug for each 
owner (and shall be doing so as I find these).

Comment 4

[John Bandhauer]

Bruce, I take it your intent is to just point these out without making 
specific judgements about correctness or the proper fix?

In both of the cases so far these are 'good' casts. The fix will be to replace 
the plain C cast with a NS_REINTERPRET_CAST. This will quiet the warning, right?

I hope that the people getting this type of bug report understand the importance 
of using an NS_STATIC_CAST where appropriate in place of a plain C cast. As it 
happens these are both reinterpret_cast kind of instances.

Thanks! 
John.

Comment 5

[Bruce Mitchener]

well, I've not reported anything that wasn't casting between C++ objects (I 
haven't reported anything that was using structs.)

I'm personally not clear any longer on the difference between a NS_STATIC_CAST 
and an NS_REINTERPRET_CAST, so I'm not making judgements on how these should be 
corrected.

Comment 6

[John Bandhauer]

FWIW, reinterpret_cast is pretty much like a plain C cast - it makes no change 
to the pointer value. However, a static_cast might actually insert some code to 
modify the pointer value. This is because an object might implement nsIFoo and 
nsIBar where those two interfaces have no inheritance relationship between them. 
So the 'nsIFoo part' of the object may actually have a different pointer value 
than the 'nsIBar part' of the object. The static_cast is made where the actual 
layout of the object is known and code to do a runtime pointer adjustment is 
automatically inserted if necessary.

Comment 7

[John Bandhauer]

I have a patch for the second problem (in xpcconvert.cpp). But the first warning 
(in xptinfo.h) can not be fixed with a reinterpret_cast - the C++ compiler 
just won't buy it because the type declaration of one of the types is not 
explicit enough at that point in the code to make the C++ compiler happy. This 
particular plain C cast is going to have to stay. I'll check in the updated 
comment about the cast in the code.

Thanks!

Comment 8

[John Bandhauer]

I checked in the fix for the cast in xpcconvert.cpp. The one in xptinfo.h is 
just going to have to stay.

Comment 9

[Phil Schwartau]

Verifying -