<br><br><div>On Wed Jan 01 2014 at 8:20:03 PM, Alp Toker <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>> wrote:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 02/01/2014 04:04, Chandler Carruth wrote:<br>
> On Wed, Jan 1, 2014 at 10:28 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br>
> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
><br>
><br>
>     On 02/01/2014 02:52, Chandler Carruth wrote:<br>
>><br>
>>     On Wed, Jan 1, 2014 at 5:35 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br>
>>     <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
>><br>
>>             & is there a reason we're not using the<br>
>>             LLVM_STATIC_ASSERT macro here?<br>
>><br>
>><br>
>>         Requires LLVM_ENABLE_CXX11 which isn't yet enabled on most<br>
>>         (any?) of the builders.<br>
>><br>
>>         I'd just wait until C++11 and change the lot.<br>
>><br>
>><br>
>>     Did you check the definition? We provide a fallback if the<br>
>>     compiler doesn't support static_assert.<br>
><br>
>     Sure, in fact I wrote the current definition of LLVM_STATIC_ASSERT<br>
>     (or at least made the changes adding compiler checks in r198142<br>
>     and r198255).<br>
><br>
>     There is no fallback..<br>
><br>
>     |/// \macro LLVM_STATIC_ASSERT||<br>
>     ||/// \brief Expands to C/C++'s static_assert on compilers which<br>
>     support it.||<br>
>     ||#if __has_feature(cxx_static_<u></u>assert) || \||<br>
>     ||    defined(__GXX_EXPERIMENTAL_<u></u>CXX0X__) || LLVM_MSC_PREREQ(1600)||<br>
>     ||# define LLVM_STATIC_ASSERT(expr, msg) static_assert(expr, msg)||<br>
>     ||#elif __has_feature(c_static_assert)<u></u>||<br>
>     ||# define LLVM_STATIC_ASSERT(expr, msg) _Static_assert(expr, msg)||<br>
>     ||#else||<br>
>     ||# define LLVM_STATIC_ASSERT(expr, msg)||<br>
>     ||#endif|<br>
><br>
>     The macro expands to nothing in the default build configuration:<br>
><br>
><br>
> That is the fallback to which I refer. What I'm saying is that you can<br>
> directly use LLVM_STATIC_ASSERT, you don't need to guard uses of it<br>
> around anything to do with C++11. And that seems fine, especially in a<br>
> unit test.<br>
><br>
> I did misread it on the first pass -- it tests for the feature<br>
> static_assert, but not the extension static_assert, but that (and<br>
> making it actually useful with GCC) is an orthogonal discussion.<br>
><br>
> My real point was that I agreed with Dave here -- it seems it would be<br>
> much better to use LLVM_STATIC_ASSERT in this test rather than rolling<br>
> our own C++98 variant. If we want to make the LLVM_STATIC_ASSERT<br>
> better so that it fires in more build configuraitons, awesome, but I<br>
> don't think its current weakness is a good argument for not using it.<br>
<br>
Totally. That's why I said "Stopgap measure until we can just use<br>
static_assert()" in my commit message.<br>
<br>
Nevertheless the fallback you suggest SGTM and I've added it to<br>
Compiler.h LLVM_STATIC_ASSERT() in r198292 so we can have it both ways.<br>
<br>
As for porting the assert in question, leaving that to someone who's<br>
more familiar with the IR unit tests.<br></blockquote><div><br></div><div>Eh - seems like a simple/mechanical enough change. I've committed it myself in r198330. I verified that it works (inverting the condition does cause the static assert to fail in a C++11 build).<br>
<br>As Chandler said - if anyone cares greatly about this failing in a 98 build they can improve the LLVM_STATIC_ASSERT machinery themselves. But given that we have many bots and developers working with C++11 builds, I expect any failure would be picked up soon enough not to matter much.<br>
<br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Alp.<br>
<br>
<br>
><br>
>     |lib/Lex/PPMacroExpansion.cpp: .Case("cxx_static_assert",<br>
>     LangOpts.CPlusPlus11)|<br>
>     |<br>
>     |||<br>
>>     We also test for support of static_assert in a more fine grain<br>
>>     way than merely that C++11 is enabled which should allow the<br>
>>     macro to work on any build using a modern (last 2 years) Clang<br>
>>     host compiler.<br>
><br>
>     Where?<br>
><br>
>     grep 'static_assert' through the LLVM source tree only finds the<br>
>     single line above.<br>
><br>
>     Alp.<br>
><br>
><br>
>><br>
>>     -Chandler<br>
><br>
>     --<br>
>     <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
>     the browser experts<br>
><br>
><br>
<br>
--<br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</blockquote>