On Aug 24, 2009, at 3:27 AM, Boaz Harrosh 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?
In addition to being consistent with xdr_{encode,decode}_hyper, this
adds appropriate static data type checking, which we want.
All in favor say I. (And even if you don't, I know what you all
think ;-))
Aye.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
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
