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
