| Message ID | CAFk2RUaDq4ga1D-LcdOJzcn+UbF8m-YzeJRYpo3rULKtS=9qRw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 21 Mar 2017, Ville Voutilainen wrote: > On 20 March 2017 at 04:27, Jason Merrill <jason@redhat.com> wrote: >> On Sun, Mar 19, 2017 at 6:19 PM, Ville Voutilainen >> <ville.voutilainen@gmail.com> wrote: >>> I ran the tests for g++.dg/init thus far. Does this patch make sense? >> >> The condition needs to be a lot more specific: DR 1748 only applies to >> the non-allocating forms in [new.delete.placement], not to other >> placement allocation functions. > > Round 2: > > The new tests tested on Linux-x64, finishing testing with the full suite on > Linux-PPC64. > > 2017-03-20 Ville Voutilainen <ville.voutilainen@gmail.com> > > gcc/ > > PR c++/35878 > * cp/init.c (build_new_1): Don't do a null check for > a namespace-scope non-replaceable placement new > in C++17 mode unless -fcheck-new is provided. It looks strange to me. Why not change the definition of check_new instead of changing the condition that uses it? One of the tests for flag_check_new is redundant. In C++17 mode, you test for NULL return from throwing operator new, why? This is a DR, doesn't it mean that it should apply to all modes? Or is the hope that limiting it to an experimental mode might let it pass in stage 4?
On Mon, Mar 20, 2017 at 7:41 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Tue, 21 Mar 2017, Ville Voutilainen wrote: > >> On 20 March 2017 at 04:27, Jason Merrill <jason@redhat.com> wrote: >>> On Sun, Mar 19, 2017 at 6:19 PM, Ville Voutilainen >>> <ville.voutilainen@gmail.com> wrote: >>>> I ran the tests for g++.dg/init thus far. Does this patch make sense? >>> >>> The condition needs to be a lot more specific: DR 1748 only applies to >>> the non-allocating forms in [new.delete.placement], not to other >>> placement allocation functions. >> >> Round 2: >> >> The new tests tested on Linux-x64, finishing testing with the full suite on >> Linux-PPC64. >> >> 2017-03-20 Ville Voutilainen <ville.voutilainen@gmail.com> >> >> gcc/ >> PR c++/35878 >> * cp/init.c (build_new_1): Don't do a null check for >> a namespace-scope non-replaceable placement new >> in C++17 mode unless -fcheck-new is provided. > > It looks strange to me. Why not change the definition of check_new instead > of changing the condition that uses it? Agreed. Also, let's factor the new tests out into a function, say non_allocating_fn_p. > In C++17 mode, you test for NULL return from throwing operator > new, why? This is a DR, doesn't it mean that it should apply to all modes? > Or is the hope that limiting it to an experimental mode might let it pass in > stage 4? Bingo. Jason
diff --git a/gcc/cp/init.c b/gcc/cp/init.c index 8bfcbde..cd34141 100644 --- a/gcc/cp/init.c +++ b/gcc/cp/init.c @@ -3450,7 +3450,21 @@ build_new_1 (vec<tree, va_gc> **placement, tree type, tree nelts, rval = TARGET_EXPR_INITIAL (alloc_expr); else { - if (check_new) + /* See DR 1748. Skip the null check in C++17 mode for non-replaceable + namespace-scope placement new forms unless the user explicitly + asks for the check. + TODO: Skip the null check in other modes as well. */ + if ((check_new && (cxx_dialect <= cxx14)) + || flag_check_new + || ((cxx_dialect > cxx14) + && !(DECL_NAMESPACE_SCOPE_P (alloc_fn) + && TREE_CHAIN (TYPE_ARG_TYPES (TREE_TYPE (alloc_fn))) + && (TREE_VALUE (TREE_CHAIN + (TYPE_ARG_TYPES (TREE_TYPE (alloc_fn)))) + == ptr_type_node) + && (TREE_CHAIN (TREE_CHAIN + (TYPE_ARG_TYPES (TREE_TYPE (alloc_fn)))) + == void_list_node)))) { tree ifexp = cp_build_binary_op (input_location, NE_EXPR, alloc_node, diff --git a/gcc/testsuite/g++.dg/init/pr35878_1.C b/gcc/testsuite/g++.dg/init/pr35878_1.C new file mode 100644 index 0000000..b45c009 --- /dev/null +++ b/gcc/testsuite/g++.dg/init/pr35878_1.C @@ -0,0 +1,21 @@ +// { dg-options "-O2 --std=gnu++11" } +// { dg-do compile } +// { dg-final { scan-assembler "test.*%rdi, %rdi" { target i?86-*-* x86_64-*-* } } } +#include <new> +#include <utility> + +struct s1{ + int a; + int b; + int c; +}; + +void f1 (s1 * v, s1&& s) +{ + new (v) s1(std::move(s)); +} + +void f2 (s1 * v, s1&& s) +{ + *v = std::move(s); +} diff --git a/gcc/testsuite/g++.dg/init/pr35878_2.C b/gcc/testsuite/g++.dg/init/pr35878_2.C new file mode 100644 index 0000000..0664494 --- /dev/null +++ b/gcc/testsuite/g++.dg/init/pr35878_2.C @@ -0,0 +1,21 @@ +// { dg-options "-O2 --std=gnu++17 -fcheck-new" } +// { dg-do compile } +// { dg-final { scan-assembler "test.*%rdi, %rdi" { target i?86-*-* x86_64-*-* } } } +#include <new> +#include <utility> + +struct s1{ + int a; + int b; + int c; +}; + +void f1 (s1 * v, s1&& s) +{ + new (v) s1(std::move(s)); +} + +void f2 (s1 * v, s1&& s) +{ + *v = std::move(s); +} diff --git a/gcc/testsuite/g++.dg/init/pr35878_3.C b/gcc/testsuite/g++.dg/init/pr35878_3.C new file mode 100644 index 0000000..8a5614f --- /dev/null +++ b/gcc/testsuite/g++.dg/init/pr35878_3.C @@ -0,0 +1,21 @@ +// { dg-options "-O2 --std=gnu++17" } +// { dg-do compile } +// { dg-final { scan-assembler-not "test.*%rdi, %rdi" { target i?86-*-* x86_64-*-* } } } +#include <new> +#include <utility> + +struct s1{ + int a; + int b; + int c; +}; + +void f1 (s1 * v, s1&& s) +{ + new (v) s1(std::move(s)); +} + +void f2 (s1 * v, s1&& s) +{ + *v = std::move(s); +}