Hi Peff,
On 2015-01-23 13:23, Jeff King wrote:
> On Fri, Jan 23, 2015 at 12:48:29PM +0100, Johannes Schindelin wrote:
>
>> Pointed out by Michael Blume. Jeff King provided the pointer to a commit
>> fixing the same issue elsewhere in the Git source code.
>
> It may be useful to reference the exact commit (3ce3ffb8) to help people
> digging in the history (e.g., if we decide there is a better way to shut
> up this warning and we need to find all the places to undo the
> brain-damage).
Good point, thanks!
>> - for (i = 0; i < FSCK_MSG_MAX; i++) {
>> + for (i = FSCK_MSG_MIN + 1; i < FSCK_MSG_MAX; i++) {
>
> Ugh. It is really a shame how covering up this warning requires
> polluting so many places. I don't think we have a better way, though,
> aside from telling people to use -Wno-tautological-compare (and I can
> believe that it _is_ a useful warning in some other circumstances, so it
> seems a shame to lose it).
>
> 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.
But it does complicate the papering over Clang's overzealous warning, so I could live with removing the check altogether.
On the other hand, I could do something even easier:
-- 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?
> I do not see either side of the bounds check here:
>
>> + if (options->msg_severity &&
>> + msg_id > FSCK_MSG_MIN && msg_id < FSCK_MSG_MAX)
>
> as really doing anything. Any code which triggers it must already cause
> undefined behavior, I think (with the exception of "msg_id == FSCK_MSG_MAX",
> but presumably that is something we never expect to happen, either).
Yep, it should not be triggered at all, but as I said, it is a nice defensive programming measure to avoid segmentation faults in case of a bug.
Ciao,
Dscho
--
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