[patch] Fix pr16247

[Eli Friedman]

On Wed, Jun 19, 2013 at 9:36 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Tue, Jun 18, 2013 at 11:08 AM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > On Tue, Jun 18, 2013 at 6:57 AM, Rafael Espíndola
> > <rafael.espindola at gmail.com> wrote:
> >>> Suppose overloading completely ignores whether a declaration is extern
> "C",
> >>> and suppose you have a hashtable which returns the extern "C" decl for
> a
> >>> given identifier.  After you conclude foo(int) overloads foo(), you
> look in
> >>> the hashtable and say "oops, there was already a declaration with that
> >>> name".  (I haven't really thought through whether this is a good idea.)
> >>
> >> The case I don't understand is
> >>
> >>
> >> extern "C" {
> >>   static void foo() { }
> >>   void foo(int x) {}
> >> }
> >>
> >> Overload in this proposal will decide that the second foo is an
> >> overload, we see that it is extern C, add it to the hash and now,
> >> where do we find the first foo? Note that it is *not* extern C
> >> according to the standard and our current implementation.
> >
> > There are three separate lookups relevant here:
> >
> > 1) Normal redeclaration lookup
> > 2) When declaring an extern "C" function that is not yet a
> > redeclaration, look for extern "C" functions in other scopes
> > 3) When declaring an extern "C" function that is (still) not yet a
> > redeclaration, error if there is a declaration of that name in the
> > global namespace
> >
> > We already do (2), but miss some cases because our map of extern "C"
> > declarations is incomplete (this causes various bugs regardless of
> > whether we allow internal linkage declarations to be extern "C").
> >
> > I think you're asking how we do (3). The answer is that we currently
> > miss this check if the declarations are not in the same scope, and
> > with my suggestion, would miss it in all cases. Again, this is
> > something we should fix regardless.
>
> Attached patch deals with all of the above, and fixes PR16247.
>
> This is slightly complicated by us using LocallyScopedExternCDecls for
> two completely different things. Prior to this change:
>  * In C, we use it to track function-scope extern declarations
>  * In C++, we fail to track function-scope extern declarations
> correctly (see PR7560) and instead used the map to track
> function-scope extern "C" declarations.
>
> After this change, the state in C is the same, but in C++, we use the
> map to track *all* extern "C" declarations. (This change doesn't fix
> PR7560, for which we should probably treat local extern variables the
> same way we handle injected friend declarations.)
>

If I'm understanding your intent correctly, the direction you want to go in
is to eventually stop using LocallyScopedExternCDecls in C altogether; am I
following this correctly?

It would be nice to add a comment to make it obvious
that checkGlobalOrExternCConflict is only used in C++.

Would it be worth trying to optimize for the "simple" case of an extern "C"
function whose redecl context is the translation unit?  It looks like the
code ends up doing an unnecessary lookup.

Otherwise, LGTM.

-Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130628/da253731/attachment.html>