| Message ID | 20181111062312.16342-4-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fast export and import fixes and features | expand |
On Sat, Nov 10, 2018 at 10:23:05PM -0800, Elijah Newren wrote: > ABORT and ERROR happen to have the same value, but come from differnt > enums. Use the one from the correct enum. Yikes. :) This is a good argument for naming these SIGNED_TAG_ABORT, etc. But this is obviously an improvement in the meantime. -Peff
On Sun, Nov 11 2018, Jeff King wrote: > On Sat, Nov 10, 2018 at 10:23:05PM -0800, Elijah Newren wrote: > >> ABORT and ERROR happen to have the same value, but come from differnt >> enums. Use the one from the correct enum. > > Yikes. :) > > This is a good argument for naming these SIGNED_TAG_ABORT, etc. But this > is obviously an improvement in the meantime. In C enum values aren't the types of the enum, but I'd thought someone would have added a warning for this: #include <stdio.h> enum { A, B } foo = A; enum { C, D } bar = C; int main(void) { switch (foo) { case C: puts("A"); break; case B: puts("B"); break; } } But none of the 4 C compilers (gcc, clang, suncc & xlc) I have warn about it. Good to know.
On Sun, Nov 11, 2018 at 9:10 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Sun, Nov 11 2018, Jeff King wrote: > > > On Sat, Nov 10, 2018 at 10:23:05PM -0800, Elijah Newren wrote: > > > >> ABORT and ERROR happen to have the same value, but come from differnt > >> enums. Use the one from the correct enum. > > > > Yikes. :) > > > > This is a good argument for naming these SIGNED_TAG_ABORT, etc. But this > > is obviously an improvement in the meantime. > > In C enum values aren't the types of the enum, but I'd thought someone > would have added a warning for this: > > #include <stdio.h> > > enum { A, B } foo = A; > enum { C, D } bar = C; > > int main(void) > { > switch (foo) { > case C: > puts("A"); > break; > case B: > puts("B"); > break; > } > } > > But none of the 4 C compilers (gcc, clang, suncc & xlc) I have warn > about it. Good to know. Asked GCC to implement it: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87983
On Sun, Nov 11, 2018 at 09:10:17PM +0100, Ævar Arnfjörð Bjarmason wrote: > > This is a good argument for naming these SIGNED_TAG_ABORT, etc. But this > > is obviously an improvement in the meantime. > > In C enum values aren't the types of the enum, but I'd thought someone > would have added a warning for this: > > #include <stdio.h> > > enum { A, B } foo = A; > enum { C, D } bar = C; > > int main(void) > { > switch (foo) { > case C: > puts("A"); > break; > case B: > puts("B"); > break; > } > } > > But none of the 4 C compilers (gcc, clang, suncc & xlc) I have warn > about it. Good to know. There is -Wenum-compare, but it does not seem to catch this (and is enabled by -Wall). It (gcc, at least) does catch: enum foo { A, B }; enum bar { C, D }; int f(enum foo x) { return x == C; } but converting that equality check to: switch (x) { case C: return 1; default: return 0; } is not (which is essentially the same as your snippet). So I think the bug / feature request is to have -Wenum-compare apply to switch statements. Clang has -Wenum-compare-switch, but I cannot seem to get it to complain about even the "==" version using -Wenum-compare. Not sure if it's buggy, or if I'm holding it wrong. This patch seems to be what we want: https://reviews.llvm.org/D36407 -Peff
diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 456797c12a..1a299c2a21 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -752,7 +752,7 @@ static void handle_tag(const char *name, struct tag *tag) tagged_mark = get_object_mark(tagged); if (!tagged_mark) { switch(tag_of_filtered_mode) { - case ABORT: + case ERROR: die("tag %s tags unexported object; use " "--tag-of-filtered-object=<mode> to handle it", oid_to_hex(&tag->object.oid));
ABORT and ERROR happen to have the same value, but come from differnt enums. Use the one from the correct enum. Signed-off-by: Elijah Newren <newren@gmail.com> --- builtin/fast-export.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)