On Thu, Mar 09, 2017 at 05:34:43PM +0100, Jakub Jelinek wrote:
> 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.
Your patch is OK as-is then.
> > 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? ;)
Ok, I'll open a PR.
Marek
