Andrew Morton wrote:
On Fri, 30 May 2008 14:10:47 +1000 Paul Mackerras <paulus@xxxxxxxxx> wrote:
What's the point here?
Readability and maintainability. Once one becomes familar with a
particular idion like this you can read it and say "ah, I know what
that's doing" rather than having to peer at every character and work it
out at each site where it happens.
As usual: a PITA now, but better long-term.
otoh,
- I think the args are backwards
I think you just admitted that the new version is less readable. At
least with an '=' operator you know which side is the thing that's
being modified. With a put_XXX function, I would have to go look up
the definition (particularly since outb et al. are also the wrong way
around, i.e. have the destination as the second argument).
Well yes, but you don't have to worry about that when reviewing because
you know the compiler will catch reversals.
Still not terribly keen about it all, but geeze the code which it is
trying to clarify is godawful:
*(__le32 *)(buf + i) = cpu_to_le32(addr);
wtf?
Then again the replacement
put_le32(addr, (__le32 *)(buf + i));
is still wtf.
..
Exactly my point. With the "improved" version, I now have to go
hunting for yet another macro in yet another header file in order
to see what the code is doing and to verify correctness.
And I (and other reviewers) are just a teensy bit less likely to
follow through with that every time, meaning we'll miss bugs.
This is somewhat akin to Linus's dislike of typedefs for structs.
It just hides what's really going on, for no obvious gain. Sorry. :)
Cheers
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
