On Aug. 24, 2009, 10:27 +0300, Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
> On 08/17/2009 07:48 PM, Trond Myklebust wrote:
>> On Mon, 2009-08-17 at 16:54 +0300, Boaz Harrosh wrote:
>>> On 08/17/2009 03:47 PM, Trond Myklebust wrote:
>>>> Adding wrappers to do this, is just unnecessarily obfuscating the code.
>>>>
>>> As I said, *sometimes* "obfuscation" is the point. If it has merits to account
>>> for human short concentration span.
>> In this case, the change was motivated by a bunch of patches which were
>> doing things like adding dummy READ32 requests immediately prior to a
>> READ_BUF() call. It was therefore abundantly clear that the obfuscation
>> was helping no-one, even though it may have made it easy to churn out
>> bad code quickly.
>>
>>>>> * I hate it that I have to count (p)"++" now to look for actual code advancements.
>>>> How is that _any_ different from counting WRITE32s?
>>>>
>>> The size for one. And mainly the symmetry with the other side of the wire.
>>>
>>> WRITEXX for every READXX. What now?
>> A
>>
>> *p++ = cpu_to_be32(foo); // WRITE32(foo)
>>
>> for every
>>
>> foo = be32_to_cpu(*p++); // READ32(foo)
>>
>
> OK I used the new stuff by now, and I'm happy with everything
> but the above. I *absolutely* insist on this changing to:
>
> 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?
>
> All in favor say I. (And even if you don't, I know what you all think ;-))
>
> Boaz
Rather than changing xdr_*code_word I'd define xdr_{en,de}code_int
which corresponds more closely to the XDR integer type.
See http://marc.info/?l=linux-nfs&m=125010800212312&w=2
Benny
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
