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

[dblaikie at gmail.com]

On Wed Jan 01 2014 at 8:20:03 PM, Alp Toker <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>> 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>> 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).

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140102/afe9df45/attachment.html>