-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 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.
Jeff Smith wrote: > Signed-off-by: Jeff Smith <[email protected]> > --- > Xtranssock.c | 14 +++++++------- > 1 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/Xtranssock.c b/Xtranssock.c > index 7106f5d..80659b3 100644 > --- a/Xtranssock.c > +++ b/Xtranssock.c > @@ -1431,19 +1431,14 @@ TRANS(SocketINETConnect) (XtransConnInfo ciptr, char > *host, char *port) > char ntopbuf[INET6_ADDRSTRLEN]; > int resetonce = 0; > #endif > - struct sockaddr_in sockname; > #ifdef XTHREADS_NEEDS_BYNAMEPARAMS > _Xgethostbynameparams hparams; > _Xgetservbynameparams sparams; > #endif > - struct hostent *hostp; > - struct servent *servp; > - unsigned long tmpaddr; > #ifdef X11_t > char portbuf[PORTBUFSIZE]; > #endif > > - long tmpport; > char hostnamebuf[256]; /* tmp space */ > > PRMSG (2,"SocketINETConnect(%d,%s,%s)\n", ciptr->fd, host, port); > @@ -1467,7 +1462,7 @@ TRANS(SocketINETConnect) (XtransConnInfo ciptr, char > *host, char *port) > > if (is_numeric (port)) > { > - tmpport = X_TCP_PORT + strtol (port, (char**)NULL, 10); > + long tmpport = X_TCP_PORT + strtol (port, (char**)NULL, 10); > sprintf (portbuf, "%lu", tmpport); > port = portbuf; If tmpport is only assigned to once, make it const. > } > @@ -1619,6 +1614,11 @@ TRANS(SocketINETConnect) (XtransConnInfo ciptr, char > *host, char *port) > } > #else > { > + struct sockaddr_in sockname; 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. > + struct hostent *hostp; > + struct servent *servp; 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. > + unsigned long tmpaddr; > + > /* > * Build the socket name. > */ > @@ -1680,7 +1680,7 @@ TRANS(SocketINETConnect) (XtransConnInfo ciptr, char > *host, char *port) > } > sockname.sin_port = htons (servp->s_port); > } else { > - tmpport = strtol (port, (char**)NULL, 10); > + long tmpport = strtol (port, (char**)NULL, 10); > if (tmpport < 1024 || tmpport > USHRT_MAX) > return TRANS_CONNECT_FAILED; > sockname.sin_port = htons (((unsigned short) tmpport)); Same comment as above RE: const. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkun4z8ACgkQX1gOwKyEAw/w7gCeJ36aexH0tbyRJsPYOIWhnvfT jjEAn1JQ4b79U82TF3WDe7FDO3aAcUhn =Wx5y -----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
