[libc++] use constexpr for sizeof comparisons

[ldionne]

Summary

This fixes a spurious placement-new error from GCC.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70834

Sponsored by: The FreeBSD Foundation

[ldionne]

include/functional

1769–1773

The purpose of _LIBCPP_CONSTEXPR_AFTER_CXX14 isn't to be used that way, it's meant for Standard Library facilities that were marked constexpr in C++17. I think we should have a macro specifically for constructing an if constexpr when supported.

[asomers]

include/functional

1769–1773

How about something like this:

#ifdef __cpp_if_constexpr
#define __IF_CONSTEXPR constexpr
#else
#define __IF_CONSTEXPR
#endif

[ldionne]

include/functional

1769–1773

Yes, except it should be #define _LIBCPP_IF_CONSTEXPR. I'm also unsure about this whole thing: for anything that requires if constexpr for correctness, we can't use _LIBCPP_IF_CONSTEXPR (because it's required for correctness). So this will only be useful for cases where we want to use if constexpr instead of just if as a QOI matter, and it's not clear to me this is worth introducing a macro. So I'm kind of ambivalent between _LIBCPP_IF_CONSTEXPR and just having the #if __cpp_if_constexpr check right there in the code. Thoughts?

[asomers]

• Use __cpp_if_constexpr.

[ldionne]

Looking at this again and the corresponding GCC bug report, I think we should either:

• not fix this issue -- this warning shouldn't bubble up since we mark our headers as system headers, or
• disable -Wplacement-new around that code with a #pragma, and reference the GCC bug in a comment.

[asomers]

In D65034#1941477, @ldionne wrote:

Looking at this again and the corresponding GCC bug report, I think we should either:

• not fix this issue -- this warning shouldn't bubble up since we mark our headers as system headers, or
• disable -Wplacement-new around that code with a #pragma, and reference the GCC bug in a comment.

Well, the warning does bubble up. I ran into this when using GCC to build FreeBSD for riscv64. The only way I could get it to build was by adding -Wno-placement-new -Wno-attributes. See https://svnweb.freebsd.org/base/head/tests/sys/fs/fusefs/Makefile?r1=350162&r2=350163& . And while we could just disable that warning, that doesn't feel right, because the GCC people don't consider this a bug at all. They say "if constexpr is of course the right solution here, for certain".

include/functional

1769–1773

Whether or not to introduce a macro is strictly a stylistic matter, and you're the one whose opinion on style matters. I'll do it as you suggest.

[ldionne]

In D65034#1941567, @asomers wrote:

In D65034#1941477, @ldionne wrote:

Looking at this again and the corresponding GCC bug report, I think we should either:

• not fix this issue -- this warning shouldn't bubble up since we mark our headers as system headers, or
• disable -Wplacement-new around that code with a #pragma, and reference the GCC bug in a comment.

Well, the warning does bubble up. I ran into this when using GCC to build FreeBSD for riscv64.

Can you show me how you're building the file that produces a warning? Normally, our headers shouldn't bubble up warnings cause we use the #pragma system header on GCC. If that doesn't work, then either their warning doesn't listen to the pragma, or something else is happening. Either way, I'd like to understand before we change any code.

And while we could just disable that warning, that doesn't feel right, because the GCC people don't consider this a bug at all.

I agree that turning the warning off completely isn't the solution.

They say "if constexpr is of course the right solution here, for certain".

On the Bug report, the GCC folks don't say that, Matt Godbolt does. The GCC developer (Martin Sebor IIUC) says that the warning isn't smart enough at the moment, which is indeed what it looks to me.

[asomers]

In D65034#1941584, @ldionne wrote:

In D65034#1941567, @asomers wrote:

In D65034#1941477, @ldionne wrote:

Looking at this again and the corresponding GCC bug report, I think we should either:

• not fix this issue -- this warning shouldn't bubble up since we mark our headers as system headers, or
• disable -Wplacement-new around that code with a #pragma, and reference the GCC bug in a comment.

Well, the warning does bubble up. I ran into this when using GCC to build FreeBSD for riscv64.

Can you show me how you're building the file that produces a warning? Normally, our headers shouldn't bubble up warnings cause we use the #pragma system header on GCC. If that doesn't work, then either their warning doesn't listen to the pragma, or something else is happening. Either way, I'd like to understand before we change any code.

Here's the command line that built the offending file:

/usr/local/bin/riscv64-unknown-freebsd13.0-g++  --sysroot=/scratch/tmp/asomers/obj/home/asomers/freebsd/base/head/riscv.riscv64/tmp -B/usr/local/riscv64-unknown-freebsd13.0/bin/  -O2 -pipe -march=rv64imafdc -mabi=lp64d -g -Wno-format-zero-length -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter -Wpointer-arith -Wreturn-type -Wcast-qual -Wwrite-strings -Wswitch -Wshadow -Wunused-parameter -Wcast-align -Wchar-subscripts -Winline -Wredundant-decls -Wno-error=address -Wno-error=array-bounds -Wno-error=attributes -Wno-error=bool-compare -Wno-error=cast-align -Wno-error=clobbered -Wno-error=deprecated-declarations -Wno-error=enum-compare -Wno-error=extra -Wno-error=inline -Wno-error=logical-not-parentheses -Wno-error=strict-aliasing -Wno-error=uninitialized -Wno-error=unused-but-set-variable -Wno-error=unused-function -Wno-error=unused-value -Wno-error=nonnull-compare -Wno-error=shift-negative-value -Wno-error=tautological-compare -Wno-error=unused-const-variable -Wno-error=bool-operation -Wno-error=deprecated -Wno-error=expansion-to-defined -Wno-error=format-overflow -Wno-error=format-truncation -Wno-error=implicit-fallthrough -Wno-error=int-in-bool-context -Wno-error=memset-elt-size -Wno-error=noexcept-type -Wno-error=nonnull -Wno-error=pointer-compare -Wno-error=stringop-overflow -Wno-error=aggressive-loop-optimizations -Wno-error=cast-function-type -Wno-error=catch-value -Wno-error=multistatement-macros -Wno-error=restrict -Wno-error=sizeof-pointer-memaccess -Wno-error=stringop-truncation  -I/home/asomers/freebsd/base/head/tests -I/home/asomers/freebsd/base/head/sys/fs/fuse -I/home/asomers/freebsd/base/head/sbin/mount -I/scratch/tmp/asomers/obj/home/asomers/freebsd/base/head/riscv.riscv64/tmp/usr/include/private -std=c++14       -c /home/asomers/freebsd/base/head/tests/sys/fs/fusefs/mockfs.cc -o mockfs.o
... Some other warnings ...

/scratch/tmp/asomers/obj/home/asomers/freebsd/base/head/riscv.riscv64/tmp/usr/include/c++/v1/functional:1774:36: error: placement new constructing an object of type '_Fun' {aka 'std::__1::__function::__func<ReturnImmediate(std::__1::function<void(const mockfs_buf_in&, mockfs_buf_out&)>)::<lambda(auto:5&, auto:6&)>, std::__1::allocator<ReturnImmediate(std::__1::function<void(const mockfs_buf_in&, mockfs_buf_out&)>)::<lambda(auto:5&, auto:6&)> >, void(const mockfs_buf_in&, std::__1::vector<std::__1::unique_ptr<mockfs_buf_out> >&)>'} and size '64' in a region of type 'std::__1::aligned_storage<24>::type' and size '32' [-Werror=placement-new=]
                     ::new ((void*)&__buf_) _Fun(_VSTD::move(__f), _Alloc(__af));

[ldionne]

In D65034#1943506, @asomers wrote:

Here's the command line that built the offending file:

[...]

You are asking for the warning by using -Wsystem-headers. I think we should either not do anything or push/pop an ignore pragma for the warning around the offending code.

[dim]

FYI FreeBSD is one of the few OSes that enables -Wsystem-headers by default, *and* uses libc++ as its default C++ library. So we've already submitted a bunch of warning fixes for libc++.

That said, sometimes it is not possible to avoid warnings in libc++ headers, for example when providing deprecated features, but generally pushing and popping the warning state for this works well. Maybe we can use something similar for this.

[ldionne]

I don't see us moving forward with this patch because of a warning in system headers that's not clever enough on GCC, so I'm going to abandon this to clean up the review queue.

include/functional

_LIBCPP_INLINE_VISIBILITY __value_func(_Fp&& __f, const _Alloc& __a)
         typedef allocator_traits<_Alloc> __alloc_traits;
         typedef __function::__func<_Fp, _Alloc, _Rp(_ArgTypes...)> _Fun;
         typedef typename __rebind_alloc_helper<__alloc_traits, _Fun>::type
             _FunAlloc;
 
         if (__function::__not_null(__f))
         {
             _FunAlloc __af(__a);
-            if (sizeof(_Fun) <= sizeof(__buf_) &&
+            if
+#ifdef __cpp_if_constexpr
+               constexpr
+#endif
+                (sizeof(_Fun) <= sizeof(__buf_) &&
                 is_nothrow_copy_constructible<_Fp>::value &&
                 is_nothrow_copy_constructible<_FunAlloc>::value)
             {
                 __f_ =
                     ::new ((void*)&__buf_) _Fun(_VSTD::move(__f), _Alloc(__af));
             }
             else
             {