Need reviewer for warning error fix on hprof_init.c

[David Holmes]

On 16/09/2012 1:58 AM, Kelly O'Hair wrote:
>
> On Sep 15, 2012, at 4:10 AM, David Holmes wrote:
>
>> On 15/09/2012 11:52 AM, Kelly O'Hair wrote:
>>>
>>> 7198327: Fix mac warning error in hprof_init.c
>>>
>>> http://cr.openjdk.java.net/~ohair/openjdk8/repo-jdk/webrev/src/share/demo/jvmti/hprof/hprof_init.c.sdiff.html
>>>
>>> The Mac warns on the if test for the port number being>  65535
>>> made more sense for the port to be a simple int.
>>
>> Seems simpler and more correct to just do:
>>
>>     if (port == 0 || (int)port>  65535) {
>
>
> If I can find a way to keep the cast count down, I tend to go that route.

But you had to add a cast back from int to unsigned short.

  254     fd = md_connect(hostname, (unsigned short)port);

My suggestion changes 1 line.

> I also know that compilers really don't pass 16bit quantities around, but 32bit quantities,
> so in my mind, passing an int is simpler and might avoid some implicit casting.
> I spent too many years working on compilers :^(

Don't compilers sometimes pass args via registers? You can pass two 
short args via a single int register.

> Of course I could see that I should have made it an unsigned int.
>
>
> So are you against this change?

It just seems like the wrong way to fix this. The code is already 
inconsistent in its use of types. The net_port element is an int/unint 
instead of being the "right" type for a port. So it gets cast to ushort 
and then the code that takes the ushort puts in a check incase the 
original int/uint overflowed a ushort. That in itself assumes that the 
compiler doesn't zero out the upperword of the original int/uint when 
doing the cast - and AFAICS compilers do zero it out! So the test is 
questionable in its validity. The compiler rightly complains that the 
test is not valid (ushort can not be > 65535 by definition).

If we used the right types all the way through and we trust compilers 
then the test itself should be removed. If we want to minimize the 
changes then we can appease the compiler by either changing port to be 
uint (requires three changes), or we can simply cast port to uint (not 
int - I made that mistake too!) in the test against 65535.

Your call.

Cheers,
David

> -kto
>
>>
>> David
>>
>>> -kto
>>>
>