On Thu, Mar 09, 2017 at 05:27:33PM +0100, Marek Polacek wrote:
> > That would be just for consistency with finish_struct, right?
>
> Yeah.
>
> > Not sure if we need such consistency, but I don't care that much. The point
> > to put it into start_enum was that we don't really care about the location
> > of the forward declaration after that.
> >
> > That said, there is one thing I'm wondering about:
> >
> > if (name != NULL_TREE)
> > enumtype = lookup_tag (ENUMERAL_TYPE, name, true, &enumloc);
> >
> > if (enumtype == NULL_TREE || TREE_CODE (enumtype) != ENUMERAL_TYPE)
> > {
> > enumtype = make_node (ENUMERAL_TYPE);
> > pushtag (loc, name, enumtype);
> > }
> >
> > with the optional patched spot after this. Now, if somebody does:
> > enum E;
> > enum E { A, B, C };
> > enum E { D, F };
> > then I think we'll complain about line 3 overriding previous definition
> > at line 1 (which is not right). Maybe if there is TYPE_STUB_DECL (do we
> > have it always?), we can override enumloc = DECL_SOURCE_LOCATION
> > (TYPE_STUB_DECL (enumtype));? I bet trying to change the binding ->locus
> > would be too much work.
>
> So if we set the DECL_SOURCE_LOCATION in finish_enum, we can make use of
> TYPE_STUB_DECL in start_enum for better diagnostics (it's set anytime we
> pushtag so it should always be available), so I guess I'd slightly prefer
> the finish_enum variant. But if you don't want to do it, that's fine with
> me too.
We can do it the same if done in start_enum, because it just uses enumloc
variable for that.
So e.g.
else if (TYPE_STUB_DECL (enumtype))
{
enumloc = DECL_SOURCE_LOCATION (TYPE_STUB_DECL (enumtype));
DECL_SOURCE_LOCATION (TYPE_STUB_DECL (enumtype)) = loc;
}
would achieve it too. Given that finish_enum doesn't have the location_t
argument, that looks simpler to me, but it is not a big deal either way.
> And if we decide to improve the diagnostic, we also need to fix the struct
> case:
>
> struct S;
> struct S { int i; };
> struct S { int i, j; };
>
> gives suboptimal
> ll.c:3:8: error: redefinition of ‘struct S’
> struct S { int i, j; };
> ^
> ll.c:1:8: note: originally defined here
> struct S;
> ^
>
> while clang gets it right:
> ll.c:3:8: error: redefinition of 'S'
> struct S { int i, j; };
> ^
> ll.c:2:8: note: previous definition is here
> struct S { int i; };
> ^
>
> Of course, feel free to leave those diagnostic bits for me.
Yeah, I'll happily defer that to you? ;)
Jakub
