[patch] Fix pr16247

[Richard Smith]

On Fri, Jun 28, 2013 at 11:35 AM, Eli Friedman <eli.friedman at gmail.com> wrote:
> 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?

Yes. My thought was to handle local extern declarations (in both C and
C++) in the same way we handle C++ friend injection: by treating them
as semantically being members of the enclosing scope, but in an
identifier namespace that is only searched by redeclaration lookup.
(In C++, redeclaration lookup for such names is currently broken.)

> 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.

Good idea, done.

> Otherwise, LGTM.

Thanks, r185229.