Re: [PATCH 0/3] Fix code issues spotted by clang
- From
Elijah Newren <newren@gmail.com>
- Date
- Nov 8, 2011, 16:05 UTC
- Message-ID
- <CABPp-BFm-BYPuGzp4hpH2ub_GBLWcZMKt8=rOR683_-+HFbb5A@mail.gmail.com>
- In-Reply-To
- <CACBZZX7bzFy_NCr4RTpYXXVmieaV7GrBfJgwdtCT1SwoXV_cGA@mail.gmail.com>
On Sun, Nov 6, 2011 at 5:33 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 20 quoted lines
> "builtin/fast-export.c", line 54: warning: enum type mismatch: op "="
>
> This seems to me to be a legitimate issue we introduced in
> 2d8ad46919213ebbd7bb72eb5b56cca8cc3ae07f, Elijah?
>
> We're defining an enum like this:
>
> static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;
> static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ABORT;
>
> And then doing:
>
> if (unset || !strcmp(arg, "abort"))
> tag_of_filtered_mode = ABORT; <-- Line 54
> else if (!strcmp(arg, "drop"))
> tag_of_filtered_mode = DROP;
> else if (!strcmp(arg, "rewrite"))
> tag_of_filtered_mode = REWRITE;
>
> Presumably that assignment should be "= ERROR".Looking back at that patch, "abort" was the word that made the most sense in the documentation and I was trying to make the code match. Unfortunately, that would result in "ABORT" being redefined (to the same value) so to squelch the compiler error I just went with the quick hack of changing "ABORT" in the enum definition to "ERROR", but didn't change "ABORT" later since "it had the same value anyway". Yeah, not the safest programming practice.
I probably should have just used FILTERED_TAG_ABORT and changed the signed_tag_mode enum to use SIGNED_TAG_ABORT (and then changed all references to "ABORT" to match one of those two)...or maybe somehow come up with a combined enum used for both (though WARN would only be useful fo signed_tag_mode, REWRITE would only be useful for tag_of_filtered_mode, etc.).