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

[Eric Christopher]

Thanks Evan! :)

-eric

On Thu, Apr 4, 2013 at 10:42 AM, Evan Cheng <evan.cheng at apple.com> wrote:
> Reverted: r178769.
>
> Evan
>
> On Apr 3, 2013, at 10:08 PM, Filip Pizlo <fpizlo at apple.com> wrote:
>
>
> On 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.
>
>
> OK, we've made this change on our end.
>
> This change can be rolled out.
>
>
> 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 love to see this as a long-term solution.
>
> -Filip
>
>
> - Doug
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>