On 08/24/2009 11:19 AM, Benny Halevy wrote:
> 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.
>
Sure, good, easier for us, no need to change these other places.
> See http://marc.info/?l=linux-nfs&m=125010800212312&w=2
>
Trond Myklebust wrote:
> These two functions are completely superfluous. Please just open code
> trivial cases like these...
>
> Cheers
> Trond
You can call them names, but it will not make that true. There are tons
of reasons to have them. There are hundreds of one-liner-inlines in the
Kernel, that are there because they define a language, above, the XDR
language. Cognitive tools to help us in our thinking and writing. I want
to look at a glance and see what this code intends, I do not want to
practice mental regular-expressions to say "Ha, this is 90% chance that
xdr thing".
With a well defined XDR language writing one side of the wire can practically
write the other side of the wire by itself. What so superfluous about that?
> Benny
Boaz
--
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
