On Wed, Oct 8, 2008 at 3:49 AM, Lennert Buytenhek
<buytenh@wantstofly.org> wrote:
quoted text
> On Tue, Oct 07, 2008 at 05:45:41PM -0700, Trent Piepho wrote:
>

quoted text
>> > +#define REG_READ(addr, reg)                                        \
>> > +   ({                                                      \
>> > +           int __ret;                                      \
>> > +                                                           \
>> > +           __ret = reg_read(ds, addr, reg);                \
>> > +           if (__ret < 0)                                  \
>> > +                   return __ret;                           \
>> > +           __ret;                                          \
>> > +   })
>>
>> Macro with a hidden use of a local ('ds') and a hidden return?  The
>> former is discouraged (but can sure save a lot of typing) but the
>> latter seems like it's just begging to cause a missing unlock or
>> free on an error path.
>
> Yeah, well, that was intentional (Nicolas Pitre suggested it, and I
> figured it made sense to do it).  I think there are a couple more places
> in the tree that do this, and it very much increases readability because
> you no longer need to check the return value explicitly every single
> time you do an MII access.

I agree with Trent, it only increases readability in the sense that
you have to read less when scanning over the code.  To me, that is the
least important sort of readability.  This sort of macro just creates
hidden traps.  I know it's annoying to check that return value every
time, but that's why we get paid the big bucks.  ;)

If, for some reason, the community feels this is the right way to go,
at *least* rename the macro to make it clear that it returns on an
error, and make ds an explicit parameter.

Andy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html