<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Apr 3, 2013, at 9:14 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;">On Wed, Apr 3, 2013 at 8:51 PM, Filip Pizlo <<a href="mailto:fpizlo@apple.com">fpizlo@apple.com</a>> wrote:<br><blockquote type="cite"><br>On Apr 3, 2013, at 7:03 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br><br>This seems like a pretty horrific hack.<br><br><br>I'm not sure I share your subjective assessment of this fix.<br><br></blockquote><br>That's your prerogative.<br><br><blockquote type="cite">I don't think we want to rely<br>on the preprocessor to do this sort of thing.<br><br><br>I've got no objections to not naming unused parameters in headers, in<br>fact, when I did this same sort of thing a while back I cleaned that<br>up and it seems like a much better way to go.<br><br><br>Unused parameters wasn't the only warning that came up. -Wmissing-noreturn<br>is another, and that one is trickier to fix.<br><br></blockquote><br>It is, but only slightly and could be turned off by a client instead.<br>I don't see<br>any reason to burden the llvm code base with changes for a particular client's<br>set of warnings.<br></div></blockquote><div><br></div><div>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?</div><div><br></div><div>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.</div><div><br></div><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite">My intuition is that it's unwise to expect WebKit's use of<br>-Wmissing-noreturn and -Wunused-param to be the last case of build failures<br>in LLVM clients due to unexpected uses of warning C++ flags. This change<br>ensures that downstream clients can benefit from LLVM's C API even if they<br>use these, and other, flags - since the C API headers themselves follow<br>really good hygiene (mostly by virtue of not having any function bodies, and<br>sticking to simple pure C declarations). This change also preserves<br>existing behavior for clients who use the C++ API instead of the C one, or<br>who have already side-stepped the build warnings/errors by other means.<br><br>Do you disagree with this intuition, or do you believe that there exists a<br>better long-term solution?<br><br></blockquote><br>I do disagree with your intuition.<br><br>There's no need to have a preprocessor macro to deal with this and it<br>can be solved by either making llvm clean to those warnings or<br>requiring clients to turn off those warnings on those files.<br></div></blockquote><div><br></div><div>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.</div><div><br></div><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><br>Another possibility is for you to undef cplusplus around the includes.<br>Ok, this one is pretty lame, but would work :)<br></div></blockquote><div><br></div><div>I'm trying to avoid lameness.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><br>We've fixed up various different compile warnings in the past and no<br>doubt will in the future. We currently build clean with a lot of<br>warnings and with more than just a couple compilers for our own<br>project files. I don't see anything wrong with building clean for<br>those warnings either or anything else as it comes up.<br></div></blockquote><div><br></div><div>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.</div><div><br></div><div>(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.)</div><div><br></div><div>-Filip</div><div><br></div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;"><br>-eric</div></blockquote></div><br></body></html>