Re: [PATCH] Fix NR_KEYS off-by-one error

[OGAWA Hirofumi]

Paul Jackson <pj@xxxxxxx> writes:

> > +	unsigned char s, i;
> > +	unsigned short v;
> 
> would limit s, i, and v if struct kbentry was changed to have
> larger types (short or int, say) for these.
> 
> Good point.
> 
> Would it work to use larger types for local variables s, i, and v now,
> in order to avoid the ugly macro, as in:
> 
> 	unsigned int s, i, v;

Yes.

> > Anyhow these will be warned by gcc.
> 
> If larger types, as I wrote above, were used for s, i, and v,
> then does gcc still warn?

Probably gcc not warn, I think.

> > Although overhead is insignificance, I'd hated to add overhead of this
> > test because test is not needed right now.
> 
> Code clarity matters most here.  If the code had been crystal clear
> to the casual reader, then the initial mistake, of removing the
> range checking, probably would never have occurred in the first place,
> and we human beings would have already saved more time than we can ever
> hope to save by optimizing this code.
> 
> You are absolutely correct that overhead is insignificant.
> 
> But code clarity - that is very significant <smile>.
> 
> Let all the essential details be spelled out, in the simplest
> most easily read, C statements that can be found to express it.
> 
> Each line of code we put in the kernel will be read by many people
> doing various things.  They will be more likely to have a correct
> understanding of our code if it is clear and simple, with a minimum
> of surprises.

Yes, I agree. But sorry, honestly I didn't see any big cleanup in your patch.
It's still using s/v/i... it is same readability at least for me...
-- 
OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/