[cfe-commits] r148158 - in /cfe/trunk: lib/Sema/SemaDecl.cpp lib/Sema/SemaExpr.cpp lib/Serialization/ASTWriterDecl.cpp test/SemaCXX/linkage.cpp

[Eli Friedman]

On Sat, Jan 14, 2012 at 4:34 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Sat, Jan 14, 2012 at 4:29 PM, Eli Friedman <eli.friedman at gmail.com>
> wrote:
>>
>> [On Sat, Jan 14, 2012 at 4:14 PM, Chandler Carruth <chandlerc at google.com>
>> wrote:
>> > On Fri, Jan 13, 2012 at 9:16 PM, Eli Friedman <eli.friedman at gmail.com>
>> > wrote:
>> >>
>> >> On Fri, Jan 13, 2012 at 7:22 PM, Matt Beaumont-Gay
>> >> <matthewbg at google.com>
>> >> wrote:
>> >> > Hi Eli,
>> >> >
>> >> > On Fri, Jan 13, 2012 at 15:41, Eli Friedman <eli.friedman at gmail.com>
>> >> > wrote:
>> >> >> Modified: cfe/trunk/test/SemaCXX/linkage.cpp
>> >> >> URL:
>> >> >>
>> >> >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/linkage.cpp?rev=148158&r1=148157&r2=148158&view=diff
>> >> >>
>> >> >>
>> >> >> ==============================================================================
>> >> >> --- cfe/trunk/test/SemaCXX/linkage.cpp (original)
>> >> >> +++ cfe/trunk/test/SemaCXX/linkage.cpp Fri Jan 13 17:41:25 2012
>> >> >> @@ -76,13 +76,15 @@
>> >> >>     struct X {
>> >> >>       int f() {
>> >> >>         extern int g();
>> >> >> -        extern int a;
>> >> >> +        // FIXME: We don't compute the correct linkage for this
>> >> >> variable
>> >> >> +        // at the moment
>> >> >> +        // extern int a;
>> >> >
>> >> > Are you planning on addressing this FIXME soon? We have some code
>> >> > which looks roughly like this, and now Clang is producing a warning
>> >> > "variable 'a' has internal linkage but is not defined".
>> >>
>> >> Per the C++11 rules, this variable clearly has internal linkage, but
>> >> the C++98 rules aren't very clear, and I'm not sure what the
>> >> gcc-compatible rules are.  If you can point me to some description, I
>> >> can implement it.
>> >
>> >
>> > I'm not sure what C++11 rules you're referring to (I just haven't read
>> > that
>> > part of the standard), but these definitely don't actually have internal
>> > linkage... Both Clang and GCC give them external linkage and C mangling.
>> > This is true even in C++11 mode, both with GCC and Clang in my tests...
>> > So
>> > this warning looks like a false positive.
>>
>> Oh, wait, nevermind, I was misreading the standard.  It's just that
>> our code for this stuff appears a bit circular because our
>> linkage-computation code distinguishes between UniqueExternal and
>> External, while the standard doesn't.
>
>
> Yea, I'm trying to remember how this works... the linkage is UniqueExternal,
> but it still gets code-genned w/ C mangling and picked up correctly from
> another TU... I think I'm actually partially responsible for this working,
> but I'll have to dig more. It's not yet clear whether the warnings
> implementation needs to change or whether we're computing the wrong linkage
> kind and just patching it up later....

I attempted to fix this in r148207; please let me know if that patch
looks sensible.

-Eli