[cfe-commits] r150128 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaDecl.cpp test/SemaCXX/function-extern-c.cpp

[Aaron Ballman]

On Thu, Feb 16, 2012 at 4:50 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Thu, Feb 9, 2012 at 3:18 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Thu, Feb 9, 2012 at 5:04 PM, Chandler Carruth <chandlerc at google.com>
>> wrote:
>> > On Thu, Feb 9, 2012 at 12:42 PM, Matt Beaumont-Gay
>> > <matthewbg at google.com>
>> > wrote:
>> >>
>> >> On a different note, what do you think about not warning when the
>> >> return type is incomplete? It's a little surprising that we warn on
>> >> this:
>> >>
>> >> struct foo;
>> >> extern "C" { struct foo get_foo(void); }
>> >> struct foo { };
>> >
>> >
>> > I think that's a bug. Incomplete struct types are relatively common as
>> > "opaque" return types.
>>
>> Oddly enough, MSVC does complain on that code.
>>
>> error C2526: 'get_foo' : C linkage function cannot return C++ class 'foo'
>>
>> It's hard to really say one way or the other, since you can make a
>> case either way.  Yes, it may be perfectly fine as an opaque type, but
>> it may also contain non-C-compatible fields which would justify the
>> warning.
>
>
> This warning is causing massive pain for our users. It has thus far found
> zero real bugs. I would like to disable it until you can change the logic to
> make it more accurate and have fewer false positives. Let me know if you
> have any strong objections to this, or if you already have the fixes in
> flight.
>
> The specific problem is that warning on the declaration is fundamentally not
> sound -- there are clearly many reasons to declare a function which uses C++
> return types but with C linkage. We've seen quite a few, some even within
> LLVM: the mangled name is much easier to deal with.

I don't think it's been finding false positives, actually.  I think
the real problem is that there's no way for the programmer to specify
their intentions.

Don't get me wrong, I can believe the warning is too chatty.  I just
don't believe that it's fundamentally unsound.  There's no way to tell
the difference between extern "C" code where the programmer simply
wants the name mangling, or extern "C" code where the programmer wants
interoperability.  The latter catches bugs, the former is noise.

> The *bug* is calling such a function from a C context. I think you should
> re-cast this warning to fire on *calls* rather than on *declarations* as
> that will be much less prone to false positives. Does this make sense?

Except that misses large swatches of real problems.  For instance,
imagine a header file declaring extern "C" functions that are exported
from a library.  The warning would fire for the user of the library
instead of the author of it, which is a bit backwards.  And there are
cases where the warning wouldn't fire at all (for instance, calling a
function in a library from C# code).

I'm uncertain of a good way to deal with the signal-to-noise ratio.
Ideally, if there was a way for the programmer to convey their
intentions, we could diagnose the warning appropriately.  But I can't
think of a way to do that, so perhaps it makes sense to find a way to
reduce the noise (move it to a another warning level, perhaps)?

~Aaron