[C PATCH] Fix debug info locus of enum with previous forward declaration (PR c/79969)

Marek Polacek polacek@redhat.com
Thu Mar 9 16:38:00 GMT 2017


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



More information about the Gcc-patches mailing list