[llvm] r198273 - Silence g++ 4.9 build issue in unit tests

[Alp Toker]

On 02/01/2014 18:36, dblaikie at gmail.com wrote:
>
>
> On Wed Jan 01 2014 at 8:20:03 PM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>
>     On 02/01/2014 04:04, Chandler Carruth wrote:
>     > On Wed, Jan 1, 2014 at 10:28 PM, Alp Toker <alp at nuanti.com
>     <mailto:alp at nuanti.com>
>     > <mailto:alp at nuanti.com <mailto:alp at nuanti.com>>> wrote:
>     >
>     >
>     >     On 02/01/2014 02:52, Chandler Carruth wrote:
>     >>
>     >>     On Wed, Jan 1, 2014 at 5:35 PM, Alp Toker <alp at nuanti.com
>     <mailto:alp at nuanti.com>
>     >>     <mailto:alp at nuanti.com <mailto:alp at nuanti.com>>> wrote:
>     >>
>     >>             & is there a reason we're not using the
>     >>             LLVM_STATIC_ASSERT macro here?
>     >>
>     >>
>     >>         Requires LLVM_ENABLE_CXX11 which isn't yet enabled on most
>     >>         (any?) of the builders.
>     >>
>     >>         I'd just wait until C++11 and change the lot.
>     >>
>     >>
>     >>     Did you check the definition? We provide a fallback if the
>     >>     compiler doesn't support static_assert.
>     >
>     >     Sure, in fact I wrote the current definition of
>     LLVM_STATIC_ASSERT
>     >     (or at least made the changes adding compiler checks in r198142
>     >     and r198255).
>     >
>     >     There is no fallback..
>     >
>     >     |/// \macro LLVM_STATIC_ASSERT||
>     >     ||/// \brief Expands to C/C++'s static_assert on compilers which
>     >     support it.||
>     >     ||#if __has_feature(cxx_static_assert) || \||
>     >     ||    defined(__GXX_EXPERIMENTAL_CXX0X__) ||
>     LLVM_MSC_PREREQ(1600)||
>     >     ||# define LLVM_STATIC_ASSERT(expr, msg) static_assert(expr,
>     msg)||
>     >     ||#elif __has_feature(c_static_assert)||
>     >     ||# define LLVM_STATIC_ASSERT(expr, msg)
>     _Static_assert(expr, msg)||
>     >     ||#else||
>     >     ||# define LLVM_STATIC_ASSERT(expr, msg)||
>     >     ||#endif|
>     >
>     >     The macro expands to nothing in the default build configuration:
>     >
>     >
>     > That is the fallback to which I refer. What I'm saying is that
>     you can
>     > directly use LLVM_STATIC_ASSERT, you don't need to guard uses of it
>     > around anything to do with C++11. And that seems fine,
>     especially in a
>     > unit test.
>     >
>     > I did misread it on the first pass -- it tests for the feature
>     > static_assert, but not the extension static_assert, but that (and
>     > making it actually useful with GCC) is an orthogonal discussion.
>     >
>     > My real point was that I agreed with Dave here -- it seems it
>     would be
>     > much better to use LLVM_STATIC_ASSERT in this test rather than
>     rolling
>     > our own C++98 variant. If we want to make the LLVM_STATIC_ASSERT
>     > better so that it fires in more build configuraitons, awesome, but I
>     > don't think its current weakness is a good argument for not
>     using it.
>
>     Totally. That's why I said "Stopgap measure until we can just use
>     static_assert()" in my commit message.
>
>     Nevertheless the fallback you suggest SGTM and I've added it to
>     Compiler.h LLVM_STATIC_ASSERT() in r198292 so we can have it both
>     ways.
>
>     As for porting the assert in question, leaving that to someone who's
>     more familiar with the IR unit tests.
>
>
> 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).

Thanks David.

LLVM_STATIC_ASSERT() is enabled everywhere except pre-C++11 gcc now so 
no major loss of coverage until switchover.

Think we're nearly there, will suggest flipping the switch a week after 
the 3.4 release.

Alp.


>
> 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.
>
> - David
>
>
>     Alp.
>
>
>     >
>     >     |lib/Lex/PPMacroExpansion.cpp: .Case("cxx_static_assert",
>     >     LangOpts.CPlusPlus11)|
>     >     |
>     >     |||
>     >>     We also test for support of static_assert in a more fine grain
>     >>     way than merely that C++11 is enabled which should allow the
>     >>     macro to work on any build using a modern (last 2 years) Clang
>     >>     host compiler.
>     >
>     >     Where?
>     >
>     >     grep 'static_assert' through the LLVM source tree only finds the
>     >     single line above.
>     >
>     >     Alp.
>     >
>     >
>     >>
>     >>     -Chandler
>     >
>     >     --
>     > http://www.nuanti.com
>     >     the browser experts
>     >
>     >
>
>     --
>     http://www.nuanti.com
>     the browser experts
>

-- 
http://www.nuanti.com
the browser experts