Re: [pnfs] [PATCH RFC v2 0/21] nfs4xdr cleanup v2

[Boaz Harrosh <bharrosh@panasas.com>]

On 08/24/2009 02:56 PM, Trond Myklebust wrote:
> On Mon, 2009-08-24 at 10:27 +0300, Boaz Harrosh wrote:
>> OK I used the new stuff by now, and I'm happy with everything
>> but the above. I *absolutely* insist on this changing to:
> 
> Feel quite free to insist, but the patch isn't going in.
> 

I think your being irrational here. I'm not the first that wants this, it has
been brought up again and again on the mailing list. The big majority of nfs/xdr
programmers want this. In fact you are the *only one* who's against it.

>> 	p = xdr_encode_word(p, foo);
>> and 
>> 	p = xdr_decode_word(p, &foo);
>>
>> [xdr_{encode,decode}_word is defined differently but is only used in a couple
>>  of sunrpc files, the change of these places shall be added to this cleanup]
>>
>> I have checked this version of the definition:
>>  static inline __be32 *
>>  xdr_encode_word(__be32 *p, __u32 val)
>>  {
>> 	*p++ = cpu_to_be32(val);
>> 	return p;
>>  }
>>
>>  static inline __be32 *
>>  xdr_decode_word(__be32 *p, __u32 *valp)
>>  {
>> 	*valp = be32_to_cpu(*p++);
>> 	return p;
>>  }
>>
>> under assembly with gcc -O2 and it gives the exact same result as the open code,
>> so I do not see what can be said against it?
> 
> It is unnecessary, 

Yes it is necessary, for the reader. (And the writer).

it looks ugly, 

I think it is not, that's a matter of taste, no? The opposite is true, your
option is the ugly one.

> the latter form takes 2 pointers and
> hides an assignment, it does an unconditional pointer increment.
> 

So do all the other xdr_{de,en}code_xxx I don't see a choice. Some eggs most
be broken when making a cake. It is totally uniform with the reset of the code
which is my point. I don't want an alien looking code in the mids of very uniform
bunch. And I don't want distracting information, I want the essence stated clearly.
I don't write assembly and I don't right computer language, I write English (conforming
to computer logic).

> IOW: Just learn the meaning of 'pointer to __be32'.
> 

What? I lost you, I don't understand what you mean?

Boaz