[cfe-commits] r171263 - in /cfe/trunk: lib/Sema/SemaDecl.cpp test/SemaCXX/function-extern-c.cpp

[Hans Wennborg]

On Wed, Jan 23, 2013 at 12:13 PM, Hans Wennborg <hans at chromium.org> wrote:
> On Sun, Dec 30, 2012 at 8:40 PM, Rafael Espindola
> <rafael.espindola at gmail.com> wrote:
>> Author: rafael
>> Date: Sun Dec 30 14:40:41 2012
>> New Revision: 171263
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=171263&view=rev
>> Log:
>> Use hasCLanguageLinkage when warning about non C return types.
>>
>> Modified:
>>     cfe/trunk/lib/Sema/SemaDecl.cpp
>>     cfe/trunk/test/SemaCXX/function-extern-c.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=171263&r1=171262&r2=171263&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Sun Dec 30 14:40:41 2012
>> @@ -6266,7 +6266,7 @@
>>      // If this function is declared as being extern "C", then check to see if
>>      // the function returns a UDT (class, struct, or union type) that is not C
>>      // compatible, and if it does, warn the user.
>> -    if (NewFD->isExternC()) {
>> +    if (NewFD->hasCLanguageLinkage()) {
>>        QualType R = NewFD->getResultType();
>>        if (R->isIncompleteType() && !R->isVoidType())
>>          Diag(NewFD->getLocation(), diag::warn_return_value_udt_incomplete)
>>
>> Modified: cfe/trunk/test/SemaCXX/function-extern-c.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/function-extern-c.cpp?rev=171263&r1=171262&r2=171263&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/SemaCXX/function-extern-c.cpp (original)
>> +++ cfe/trunk/test/SemaCXX/function-extern-c.cpp Sun Dec 30 14:40:41 2012
>> @@ -38,3 +38,16 @@
>>  extern "C" A *f10( void );
>>
>>  extern "C" struct mypodstruct f12(); // expected-warning {{'f12' has C-linkage specified, but returns incomplete type 'struct mypodstruct' which could be incompatible with C}}
>> +
>> +namespace test2 {
>> +  // FIXME: we should probably suppress the first warning as the second one
>> +  // is more precise.
>> +  // For now this tests that a second 'extern "C"' is not necessary to trigger
>> +  // the warning.
>> +  struct A;
>> +  extern "C" A f(void); // expected-warning {{'f' has C-linkage specified, but returns incomplete type 'test2::A' which could be incompatible with C}}
>> +  struct A {
>> +    A(const A&);
>> +  };
>> +  A f(void);  // expected-warning {{'f' has C-linkage specified, but returns user-defined type 'test2::A' which is incompatible with C}}
>> +}
>
> Hi Rafael,
>
> As far as I can tell, your patch makes Clang warn about returning
> non-POD types from functions with C-style linkage, even if those
> functions have internal linkage. I'm not sure we want to warn in that
> case?
>
> We tripped over this in Chrome where we have some code that goes like this:
>
> extern "C" {
>   static NonPODType x() { ... }
>   void foo() { ... code that uses x() }
> }
>
> Now we get a warning about x(). Does it make sense to warn here, given
> that the function has internal linkage, and so will only be called
> from C++ code?
>
> It seems the test case you added passes already without your change to
> SemaDecl.cpp, so I'm wondering if your change was necessary?

Ping?