On Fri, Jan 23, 2015 at 01:38:17PM +0100, Johannes Schindelin wrote:
> > Unless we are willing to drop the ">= 0" check completely. I think it is
> > valid to do so regardless of the compiler's representation decision due
> > to the numbering rules I mentioned above. It kind-of serves as a
> > cross-check that we haven't cast some random int into the enum, but I
> > think we would do better to find those callsites (since they are not
> > guaranteed to work, anyway; in addition to signedness, it might choose a
> > much smaller representation).
>
> Yeah, well, this check is really more of a safety net in case I messed
> up anything; I was saved so many times by my own defensive programming
> that I try to employ it as much as I can.
Yeah, I am all in favor of defensive programming. But I am not sure that
it is defending much here, as we silently fall back to an alternate
value for the severity. Would we notice, or would that produce subtly
wrong results? IOW, would this be better as:
assert(msg_id >= 0 && msg_id < FSCK_MSG_MAX);
or something?
> -- snip --
> diff --git a/fsck.c b/fsck.c
> index 15cb8bd..8f8c82f 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -107,7 +107,7 @@ static int fsck_msg_severity(enum fsck_msg_id msg_id,
> {
> int severity;
>
> - if (options->msg_severity && msg_id >= 0 && msg_id < FSCK_MSG_MAX)
> + if (options->msg_severity && ((unsigned int) msg_id) < FSCK_MSG_MAX)
> severity = options->msg_severity[msg_id];
> else {
> severity = msg_id_info[msg_id].severity;
> -- snap --
>
> What do you think? Michael, does this cause more Clang warnings, or would it resolve the issue?
Hmm, yeah, that does not seem unreasonable, and is more localized.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html