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

[Paul Jackson <pj@sgi.com>]

Thank-you for considering my comments.

OGAWA wrote:
> because it limits the range when struct kbentry
> was changed.  (I think this change is likely)

I'm not sure I understand this comment -- but I am guessing that
you mean that:

> +	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;

However, if I have guessed incorrectly, then this idea may make no
sense.  If so, sorry about that.


> key_map is pointer, so ARRAY_SIZE() can't use.

Yes - good point.


> 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?


> 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.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373