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

[Filip Pizlo]

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.

Isn't the whole point of the C API to present a module boundary that doesn't burden clients with having to know, and build, LLVM's internals?  Or do you view LLVM's C API as being unlike other C APIs in this regard?

In particular, this doesn't burden "the llvm code base", it only burdens the headers for the C bindings.  There is a difference in scale between what this change actually does, and how you portray it in this sentence.

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

I don't want to have to keep LLVM's and WebKit's warning settings in sync, even if it only affects those files.  I want to get to a point where I can freely include LLVM headers in a bunch of places in WebKit.  Having to disable warnings (either on a per-.cpp-file basis if that file transitively includes LLVM, or by doing #pragma push or the like prior to #include <llvm-c/blah.h>, would be hugely intrusive.

> 
> Another possibility is for you to undef cplusplus around the includes.
> Ok, this one is pretty lame, but would work :)

I'm trying to avoid lameness.

> 
> 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'm sure you do, and like I said, the C API is already mostly awesome from the standpoint of being able to work out of the box.  I believe it is a progression to be able to promise to clients that including the C API won't require them to change their build configuration by much.

(They still do have to change their configuration, of course - they must define __STDC_LIMIT_MACROS, __STDC_CONSTANT_MACROS, and possibly LLVM_DO_NOT_INCLUDE_CPP_HEADERS - but adding -D flags on the command-line or #define's in a config.h file is much less intrusive than changing your entire build configuration.)

-Filip


> 
> -eric

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130403/a700ffd7/attachment.html>