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

OGAWA Hirofumi
2004-07-29T09:27:55Z

Paul Jackson <pj@sgi.com> 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@mail.parknet.co.jp>