On 9/28/20 12:30 PM, Marek Polacek wrote:
On Sat, Sep 26, 2020 at 01:22:41AM -0400, Jason Merrill wrote:+bool +ref_conv_binds_directly_p (tree type, tree expr) +{ + gcc_assert (TYPE_REF_P (type)); + conversion *conv = implicit_conversion (type, TREE_TYPE (expr), expr, + /*c_cast_p=*/false, + LOOKUP_IMPLICIT, tf_none); + return conv && !conv_binds_ref_to_prvalue (conv);You probably want to free any allocated conversions, like in can_convert_arg.I ought to free them, indeed. Apologies for missing that. Fixed: Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- This new warning can be used to prevent expensive copies inside range-based for-loops, for instance: struct S { char arr[128]; }; void fn () { S arr[5]; for (const auto x : arr) { } } where auto deduces to S and then we copy the big S in every iteration. Using "const auto &x" would not incur such a copy. With this patch the compiler will warn: q.C:4:19: warning: loop variable 'x' creates a copy from type 'const S' [-Wrange-loop-construct] 4 | for (const auto x : arr) { } | ^ q.C:4:19: note: use reference type 'const S&' to prevent copying 4 | for (const auto x : arr) { }
It's unfortunate that we seem to suggest the unnecessary change from auto to S here. Maybe just say "reference type" without printing the type?
| ^ | & As per Clang, this warning is suppressed for trivially copyable types whose size does not exceed 64B. The tricky part of the patch was how to figure out if using a reference would have prevented a copy. To that point, I'm using the new function called ref_conv_binds_directly_p. This warning is enabled by -Wall. Further warnings of similar nature should follow soon. gcc/c-family/ChangeLog: PR c++/94695 * c.opt (Wrange-loop-construct): New option. gcc/cp/ChangeLog: PR c++/94695 * call.c (ref_conv_binds_directly_p): New function. * cp-tree.h (ref_conv_binds_directly_p): Declare. * parser.c (warn_for_range_copy): New function. (cp_convert_range_for): Call it. gcc/ChangeLog: PR c++/94695 * doc/invoke.texi: Document -Wrange-loop-construct. gcc/testsuite/ChangeLog: PR c++/94695 * g++.dg/warn/Wrange-loop-construct.C: New test. --- gcc/c-family/c.opt | 4 + gcc/cp/call.c | 22 ++ gcc/cp/cp-tree.h | 1 + gcc/cp/parser.c | 68 +++++- gcc/doc/invoke.texi | 21 +- .../g++.dg/warn/Wrange-loop-construct.C | 207 ++++++++++++++++++ 6 files changed, 318 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wrange-loop-construct.C diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 7761eefd203..bbf7da89658 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -800,6 +800,10 @@ Wpacked-not-aligned C ObjC C++ ObjC++ Var(warn_packed_not_aligned) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn when fields in a struct with the packed attribute are misaligned.+Wrange-loop-construct+C++ ObjC++ Var(warn_range_loop_construct) Warning LangEnabledBy(C++ ObjC++,Wall) +Warn when a range-based for-loop is creating unnecessary copies. + Wredundant-tags C++ ObjC++ Var(warn_redundant_tags) Warning Warn when a class or enumerated type is referenced using a redundant class-key. diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 5606389f4bd..1e5fffe20ae 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -8429,6 +8429,28 @@ conv_binds_ref_to_prvalue (conversion *c) return false; }+/* True iff converting EXPR to a reference type TYPE does not involve+ creating a temporary. */ + +bool +ref_conv_binds_directly_p (tree type, tree expr) +{ + gcc_assert (TYPE_REF_P (type)); + + /* Get the high-water mark for the CONVERSION_OBSTACK. */ + void *p = conversion_obstack_alloc (0); + + conversion *conv = implicit_conversion (type, TREE_TYPE (expr), expr, + /*c_cast_p=*/false, + LOOKUP_IMPLICIT, tf_none); + bool ret = conv && !conv->bad_p && !conv_binds_ref_to_prvalue (conv); + + /* Free all the conversions we allocated. */ + obstack_free (&conversion_obstack, p); + + return ret; +} + /* Call the trivial destructor for INSTANCE, which can be either an lvalue of class type or a pointer to class type. If NO_PTR_DEREF is true and INSTANCE has pointer type, clobber the pointer rather than what it points diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index b7f5b6b399f..303e3b53e9d 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -6225,6 +6225,7 @@ extern bool sufficient_parms_p (const_tree); extern tree type_decays_to (tree); extern tree extract_call_expr (tree); extern tree build_trivial_dtor_call (tree, bool = false); +extern bool ref_conv_binds_directly_p (tree, tree); extern tree build_user_type_conversion (tree, tree, int, tsubst_flags_t); extern tree build_new_function_call (tree, vec<tree, va_gc> **, diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 8905833fbd6..6a08418e197 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -12646,6 +12646,64 @@ do_range_for_auto_deduction (tree decl, tree range_expr) } }+/* Warns when the loop variable should be changed to a reference type to+ avoid unnecessary copying. I.e., from + + for (const auto x : range) + + where range returns a reference, to + + for (const auto &x : range) + + if this version doesn't make a copy. DECL is the RANGE_DECL; EXPR is the + *__for_begin expression. + This function is never called when processing_template_decl is on. */ + +static void +warn_for_range_copy (tree decl, tree expr) +{ + if (!warn_range_loop_construct + || decl == error_mark_node) + return; + + location_t loc = DECL_SOURCE_LOCATION (decl); + tree type = TREE_TYPE (decl); + + if (from_macro_expansion_at (loc)) + return; + + if (TYPE_REF_P (type)) + { + /* TODO: Implement reference warnings. */ + return; + } + else if (!CP_TYPE_CONST_P (type)) + return; + + /* Since small trivially copyable types are cheap to copy, we suppress the + warning for them. 64B is a common size of a cache line. */ + if (TREE_CODE (TYPE_SIZE_UNIT (type)) != INTEGER_CST + || (tree_to_uhwi (TYPE_SIZE_UNIT (type)) <= 64 + && trivially_copyable_p (type))) + return; + + tree rtype = cp_build_reference_type (type, /*rval*/false); + /* If we could initialize the reference directly, it wouldn't involve any + copies. */ + if (!ref_conv_binds_directly_p (rtype, expr)) + return; + + auto_diagnostic_group d; + if (warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wrange_loop_construct,
Why not use 'loc' here? OK however you want to resolve these comments.
+ "loop variable %qD creates a copy from type %qT", + decl, type)) + { + gcc_rich_location richloc (loc); + richloc.add_fixit_insert_before ("&"); + inform (&richloc, "use reference type %qT to prevent copying", rtype); + } +} + /* Converts a range-based for-statement into a normal for-statement, as per the definition.@@ -12656,7 +12714,7 @@ do_range_for_auto_deduction (tree decl, tree range_expr) {auto &&__range = RANGE_EXPR; - for (auto __begin = BEGIN_EXPR, end = END_EXPR; + for (auto __begin = BEGIN_EXPR, __end = END_EXPR; __begin != __end; ++__begin) { @@ -12756,14 +12814,16 @@ cp_convert_range_for (tree statement, tree range_decl, tree range_expr, cp_maybe_mangle_decomp (range_decl, decomp_first_name, decomp_cnt);/* The declaration is initialized with *__begin inside the loop body. */- cp_finish_decl (range_decl, - build_x_indirect_ref (input_location, begin, RO_UNARY_STAR, - tf_warning_or_error), + tree deref_begin = build_x_indirect_ref (input_location, begin, RO_UNARY_STAR, + tf_warning_or_error); + cp_finish_decl (range_decl, deref_begin, /*is_constant_init*/false, NULL_TREE, LOOKUP_ONLYCONVERTING); if (VAR_P (range_decl) && DECL_DECOMPOSITION_P (range_decl)) cp_finish_decomp (range_decl, decomp_first_name, decomp_cnt);+ warn_for_range_copy (range_decl, deref_begin);+ return statement; }diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texiindex 3e087b6e0d9..03cb4b31b89 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -245,7 +245,7 @@ in the following sections. -Wmultiple-inheritance -Wnamespaces -Wnarrowing @gol -Wnoexcept -Wnoexcept-type -Wnon-virtual-dtor @gol -Wpessimizing-move -Wno-placement-new -Wplacement-new=@var{n} @gol --Wredundant-move -Wredundant-tags @gol +-Wrange-loop-construct -Wredundant-move -Wredundant-tags @gol -Wreorder -Wregister @gol -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol -Wno-non-template-friend -Wold-style-cast @gol @@ -3605,6 +3605,24 @@ treats the return value as if it were designated by an rvalue.This warning is enabled by @option{-Wextra}. +@item -Wrange-loop-construct @r{(C++ and Objective-C++ only)}+@opindex Wrange-loop-construct +@opindex Wno-range-loop-construct +This warning warns when a C++ range-based for-loop is creating an unnecessary +copy. This can happen when the range declaration is not a reference, but +probably should be. For example: + +@smallexample +struct S @{ char arr[128]; @}; +void fn () @{ + S arr[5]; + for (const auto x : arr) @{ @dots{} @} +@} +@end smallexample + +It does not warn when the type being copied is a trivially-copyable type whose +size is less than 64 bytes. This warning is enabled by @option{-Wall}. + @item -Wredundant-tags @r{(C++ and Objective-C++ only)} @opindex Wredundant-tags @opindex Wno-redundant-tags @@ -5274,6 +5292,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wparentheses @gol -Wpessimizing-move @r{(only for C++)} @gol -Wpointer-sign @gol +-Wrange-loop-construct @r{(only for C++)} @gol -Wreorder @gol -Wrestrict @gol -Wreturn-type @gol diff --git a/gcc/testsuite/g++.dg/warn/Wrange-loop-construct.C b/gcc/testsuite/g++.dg/warn/Wrange-loop-construct.C new file mode 100644 index 00000000000..3caf00d412f --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wrange-loop-construct.C @@ -0,0 +1,207 @@ +// PR c++/94695 +// { dg-do compile { target c++11 } } +// { dg-options "-Wrange-loop-construct" } + +#include <initializer_list> + +struct Small { + char arr[64]; +}; + +struct Big_aggr { + char arr[65]; +}; + +struct Big_triv_copy { + char arr[65]; + Big_triv_copy() { } +}; + +struct Big { + char arr[65]; + Big () = default; + Big(const Big&); +}; + +struct Foo { }; +struct Bar { + char arr[100]; + Bar(Foo); + Bar(int); + operator int(); +}; + +template<typename T> +struct It { + T operator*(); + It operator++(); + bool operator!=(const It); +}; + +template<typename T> +struct Cont { + using I = It<T>; + I begin(); + I end(); +}; + +#define TEST \ + void fn_macro() \ + { \ + Cont<Bar &> cont_bar_ref; \ + for (const Bar x : cont_bar_ref) { (void) x; } \ + } + +TEST + +Cont<Bar &>& foo (); +Cont<Bar &> foo2 (); + +void +fn1 () +{ + for (const auto x : foo () ) { (void) x; } // { dg-warning "creates a copy" } + for (const auto x : foo2 () ) { (void) x; } // { dg-warning "creates a copy" } + + Small s{}; + Small sa[5] = { }; + for (const auto x : sa) { (void) x; } + for (const auto x : { s, s, s }) { (void) x; } + + Big_aggr b{}; + Big_aggr ba[5] = { }; + for (const auto x : ba) { (void) x; } // { dg-warning "creates a copy" } + for (const auto x : { b, b, b }) { (void) x; } // { dg-warning "creates a copy" } + + Big_triv_copy bt{}; + Big_triv_copy bta[5]; + for (const auto x : bta) { (void) x; } // { dg-warning "creates a copy" } + for (const auto x : { bt, bt, bt }) { (void) x; } // { dg-warning "creates a copy" } + + Big b2; + Big ba2[5]; + for (const auto x : ba2) { (void) x; } // { dg-warning "creates a copy" } + for (const auto x : { b2, b2, b2 }) { (void) x; } // { dg-warning "creates a copy" } +} + +void +fn2 () +{ + Cont<int> cont_int; + for (const auto x : cont_int) { (void) x; } + for (const int x : cont_int) { (void) x; } + for (int x : cont_int) { (void) x; } + for (const auto &x : cont_int) { (void) x; } + for (double x : cont_int) { (void) x; } + for (const double x : cont_int) { (void) x; } + for (const Bar x : cont_int) { (void) x; } + for (Bar x : cont_int) { (void) x; } +} + +void +fn3 () +{ + Cont<int &> cont_int_ref; + for (const int x : cont_int_ref) { (void) x; } + for (int x : cont_int_ref) { (void) x; } + for (const double x : cont_int_ref) { (void) x; } + for (double x : cont_int_ref) { (void) x; } + for (const Bar x : cont_int_ref) { (void) x; } + for (Bar x : cont_int_ref) { (void) x; } +} + +void +fn4 () +{ + Cont<Bar> cont_bar; + for (const Bar x : cont_bar) { (void) x; } + for (Bar x : cont_bar) { (void) x; } + for (const int x : cont_bar) { (void) x; } + for (int x : cont_bar) { (void) x; } +} + +void +fn5 () +{ + Cont<Bar&> cont_bar_ref; + for (const Bar x : cont_bar_ref) { (void) x; } // { dg-warning "creates a copy" } + for (Bar x : cont_bar_ref) { (void) x; } + for (const int x : cont_bar_ref) { (void) x; } + for (int x : cont_bar_ref) { (void) x; } +} + +void +fn6 () +{ + Cont<Foo> cont_foo; + for (const Bar x : cont_foo) { (void) x; } + for (Bar x : cont_foo) { (void) x; } +} + +void +fn7 () +{ + Cont<Foo &> cont_foo_ref; + for (const Bar x : cont_foo_ref) { (void) x; } + for (Bar x : cont_foo_ref) { (void) x; } +} + +void +fn8 () +{ + double arr[2]; + for (const double x : arr) { (void) x; } + for (double x : arr) { (void) x; } + for (const int x : arr) { (void) x; } + for (int x : arr) { (void) x; } + for (const Bar x : arr) { (void) x; } + for (Bar x : arr) { (void) x; } +} + +void +fn9 () +{ + Foo foo[2]; + for (const Foo x : foo) { (void) x; } + for (Foo x : foo) { (void) x; } + for (const Bar x : foo) { (void) x; } + for (Bar x : foo) { (void) x; } +} + +void +fn10 () +{ + Bar bar[2] = { 1, 2 }; + for (const Bar x : bar) { (void) x; } // { dg-warning "creates a copy" } + for (Bar x : bar) { (void) x; } + for (const int x : bar) { (void) x; } + for (int x : bar) { (void) x; } +} + +template<typename T> +void +fn11 () +{ + Cont<Bar> cont_bar; + for (const Bar x : cont_bar) { (void) x; } + + Cont<Bar&> cont_bar_ref; + for (const Bar x : cont_bar_ref) { (void) x; } // { dg-warning "creates a copy" } + + Cont<T> cont_dep; + for (const T x : cont_dep) { (void) x; } +} + +template<typename T> +void +fn12 () +{ + for (const auto x : { T{} }) { (void) x; } // { dg-warning "creates a copy" } +} + +void +invoke () +{ + fn11<int> (); + fn12<Big> (); +} base-commit: e189d5c931c5d94fd7487a2d1df9454d0b124eeb
