Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 28 Jul 2004 23:15:48 -0700 | From | Paul Jackson <> | Subject | Re: [PATCH] Fix NR_KEYS off-by-one error |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| ![\](/images/icornerr.gif) |