From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jim Meyering <jim@meyering.net>
Subject: Re: [PATCH 2/3] CLD: switch network proto from UDP to TCP
Date: Mon, 03 Jan 2011 11:43:56 +0100
Message-ID: <87wrmmi7wz.fsf@meyering.net>
References: <20101231105634.GA4210@havoc.gtf.org>
	<20101231105728.GA4239@havoc.gtf.org>
	<20110102163232.78eec2f3@lembas.zaitcev.lan>
Mime-Version: 1.0
Return-path: <hail-devel-owner@vger.kernel.org>
In-Reply-To: <20110102163232.78eec2f3@lembas.zaitcev.lan> (Pete Zaitcev's
	message of "Sun, 2 Jan 2011 16:32:32 -0700")
Sender: hail-devel-owner@vger.kernel.org
List-ID: <hail-devel.vger.kernel.org>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: Pete Zaitcev <zaitcev@redhat.com>
Cc: Jeff Garzik <jeff@garzik.org>, hail-devel@vger.kernel.org

Pete Zaitcev wrote:
> On Fri, 31 Dec 2010 05:57:28 -0500
> Jeff Garzik <jeff@garzik.org> wrote:
>
>> +	struct cldc_tcp *tcp = private;
>> +	ssize_t rc;
>> +	struct ubbp_header ubbp;
>> +
>> +	memcpy(ubbp.magic, "CLD1", 4);
>> +	ubbp.op_size = (buflen << 8) | 1;
>> +#ifdef WORDS_BIGENDIAN
>> +	swab32(ubbp.op_size);
>> +#endif
>> +
>> +	rc = write(tcp->fd, &ubbp, sizeof(ubbp));
>
> Why not this:
>
> 	unsigned int n;
>
> 	n = (buflen << 8) | 1;
> 	ubbp.op_size = GUINT32_TO_LE(n);

Nice.
Avoiding those pesky in-function #ifdefs makes the code more readable.

IMHO, this is kinder still on the eyes of reviewers, since the
types of "n" and "rc" stay even closer to each definition/first-use:

  	unsigned int n = (buflen << 8) | 1;
  	ubbp.op_size = GUINT32_TO_LE(n);

        ssize_t rc = write(tcp->fd, &ubbp, sizeof(ubbp));

But if your coding guidelines require the all-vars-decl'd-at-top style
(seriously anachronistic and more prone to merge conflicts), then I guess
that's not an option...  Though you may want to reconsider the policy,
if your goal is portability:

    For the record, compilers that reject decl-after-stmt are not
    relevant any more.  At least, there are so few that I've been
    able to keep that sort of construct in coreutils for the past several
    years.  I used to maintain a c99-to-c89 patch, but removed even that
    almost two years ago.