[llvm] r178713 - Make it possible to include llvm-c without including C++ headers. Patch by Filip Pizlo.

[Eric Christopher]

On Wed, Apr 3, 2013 at 9:35 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Apr 3, 2013, at 9:14 PM, Eric Christopher <echristo at gmail.com> wrote:
>
>> On Wed, Apr 3, 2013 at 8:51 PM, Filip Pizlo <fpizlo at apple.com> wrote:
>>>
>>> On Apr 3, 2013, at 7:03 PM, Eric Christopher <echristo at gmail.com> wrote:
>>>
>>> This seems like a pretty horrific hack.
>>>
>>>
>>> I'm not sure I share your subjective assessment of this fix.
>>>
>>
>> That's your prerogative.
>>
>>> I don't think we want to rely
>>> on the preprocessor to do this sort of thing.
>>>
>>>
>>> I've got no objections to not naming unused parameters in headers, in
>>> fact, when I did this same sort of thing a while back I cleaned that
>>> up and it seems like a much better way to go.
>>>
>>>
>>> Unused parameters wasn't the only warning that came up.  -Wmissing-noreturn
>>> is another, and that one is trickier to fix.
>>>
>>
>> It is, but only slightly and could be turned off by a client instead.
>> I don't see
>> any reason to burden the llvm code base with changes for a particular client's
>> set of warnings.
>>
>>> My intuition is that it's unwise to expect WebKit's use of
>>> -Wmissing-noreturn and -Wunused-param to be the last case of build failures
>>> in LLVM clients due to unexpected uses of warning C++ flags.  This change
>>> ensures that downstream clients can benefit from LLVM's C API even if they
>>> use these, and other, flags - since the C API headers themselves follow
>>> really good hygiene (mostly by virtue of not having any function bodies, and
>>> sticking to simple pure C declarations).  This change also preserves
>>> existing behavior for clients who use the C++ API instead of the C one, or
>>> who have already side-stepped the build warnings/errors by other means.
>>>
>>> Do you disagree with this intuition, or do you believe that there exists a
>>> better long-term solution?
>>>
>>
>> I do disagree with your intuition.
>>
>> There's no need to have a preprocessor macro
>
> In the parlance of the Clang modules work, this new macro is a "configuration macro", because it's definition affects the API as defined by the header. Generally speaking, configuration macros are bad for modules (and modular APIs in general), because you don't have a fixed API vended by a given header file. What this essentially means is that, (1) one should consider very, very carefully before adding configuration macros, and (2) this will have unfortunate effects when we eventually get to using LLVM's APIs via modules.
>
>> to deal with this and it
>> can be solved by either making llvm clean to those warnings or
>> requiring clients to turn off those warnings on those files.
>
> #pragma'ing the warnings away seems like the right short-term solution.
>
>> Another possibility is for you to undef cplusplus around the includes.
>> Ok, this one is pretty lame, but would work :)
>
> __cplusplus is also a configuration macro, albeit a standardized one.
>
>> We've fixed up various different compile warnings in the past and no
>> doubt will in the future. We currently build clean with a lot of
>> warnings and with more than just a couple compilers for our own
>> project files. I don't see anything wrong with building clean for
>> those warnings either or anything else as it comes up.
>
>
> I think it's fine to fix the warnings.
>
> I think the root of the problem is that "llvm-c" is the C API, yet it conditionally includes C++ headers. If someone asks for the C API, we should *give them the C API*. If they want the C++ API, they can include additional, C++-only headers. I think that's the right long-term fix. Sure, some existing clients that use the llvm-c headers and expect the C++ API will have to add #includes, but that's a better solution (architecturally) than adding this configuration macro.
>

I would like to state that I agree with everything in this email absolutely :)

-eric