-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Jeff Smith wrote: > > From: Ian Romanick <[email protected]> > To: Jeff Smith <[email protected]> > Cc: [email protected] > Sent: Mon, March 22, 2010 4:38:08 PM > Subject: Re: [PATCH libxtrans] Clean up some 'unused variable' warnings > >> Specific comments below. Given the nature of the changes and some of my >> comments, I think these should be broken into a separate patch for each >> variable. If nothing else, it makes reviewing the changes easier. > > I had figured given their close proximity, a single patch would be fine, but > OK.
They start close, but I think I've read books shorter than that function. Given that the code is actually correct (see below), I won't complain if this comes back as a single patch. >> If tmpport is only assigned to once, make it const. > > OK. > >> I always get nervous about patches that move variable declarations in >> the presence of heavily ifdef'ed code. Whichever #if paths you don't >> test are broken. :) sockname is also used previously around lines 1534 >> and 1575 in the unpatched code. > > I already checked for other instances of the variables. For the locations you > mention, "sockname" is actually just part of a text string. Ah, you are correct. I misread the code the first time I was reviewing it. >> Both hostp and servp are used in a single if-block. I think they should >> be moved into those blocks and get the same const treatment (becoming >> 'struct hostent *const hostp') that I recommended for tmpport. > > OK. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkuoRFMACgkQX1gOwKyEAw/+2gCcDOaK3YORAE85PGv30ROgv36r vpoAmgIogctYfOxlE6UyzEff9tRQ7I56 =zFTK -----END PGP SIGNATURE----- _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
