[03/10] fast-export: use value from correct enum
diff mbox series

Message ID 20181111062312.16342-4-newren@gmail.com
State New
Headers show
Series
  • fast export and import fixes and features
Related show

Commit Message

Elijah Newren Nov. 11, 2018, 6:23 a.m. UTC
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(-)

Comments

Jeff King Nov. 11, 2018, 6:36 a.m. UTC | #1
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
Ævar Arnfjörð Bjarmason Nov. 11, 2018, 8:10 p.m. UTC | #2
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.
Ævar Arnfjörð Bjarmason Nov. 12, 2018, 9:12 a.m. UTC | #3
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
Jeff King Nov. 12, 2018, 11:31 a.m. UTC | #4
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

Patch
diff mbox series

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));