On 9/24/20 6:05 PM, Marek Polacek via Gcc-patches wrote:
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) { } | ^ | & 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. I've used perform_implicit_conversion to perform the imaginary conversion. Then if the conversion doesn't have any side-effects, I assume it does not call any functions or create any TARGET_EXPRs, and is just a simple assignment like this one: const T &x = (const T &) <__for_begin>; But it can also be a CALL_EXPR: x = (const T &) Iterator<T&>::operator* (&__for_begin) which is still fine -- we just use the return value and don't create any copies. This warning is enabled by -Wall. Further warnings of similar nature should follow soon.
I've always thought a warning like this would be useful when passing large objects to functions by value. Is adding one for these cases what you mean by future warnings? For the range loop, I wonder if more could be done to elide the copy and avoid the warning when it isn't really necessary. For instance, for trivially copyable types like in your example, since x is const, modifying it would be undefined, and so when we can prove that the original object itself isn't modified (e.g., because it's declared const, or because it can't be accessed in the loop), there should be no need to make a copy on each iteration. Using a reference to the original object should be sufficient. Does C++ rule out such an optimization? About the name of the option: my first thought was that it was about the construct known as the range loop, but after reading your description I wonder if it might actually primarily be about constructing expensive copies and the range loop is incidental. (It's impossible to tell from the Clang manual because its way of documenting warning options is to show examples of their text.) Then again, I see it's related to -Wrange-loop-analysis so that suggests it is mainly about range loops, and that there may be a whole series of warnings and options related to it. Can you please shed some light on that? (E.g., what are some of the "further warnings of similar nature" about?) I think it might also be helpful to expand the documentation a bit to help answer common questions (I came across the following post while looking it up: https://stackoverflow.com/questions/50066139). Martin
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? gcc/c-family/ChangeLog: PR c++/94695 * c.opt (Wrange-loop-construct): New option. gcc/cp/ChangeLog: PR c++/94695 * 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/parser.c | 77 ++++++- gcc/doc/invoke.texi | 21 +- .../g++.dg/warn/Wrange-loop-construct.C | 207 ++++++++++++++++++ 4 files changed, 304 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/parser.c b/gcc/cp/parser.c index fba3fcc0c4c..d233279ac62 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -12646,6 +12646,73 @@ 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); + /* See what it would take to convert the expr if we used a reference. */ + expr = perform_implicit_conversion (rtype, expr, tf_none); + if (!TREE_SIDE_EFFECTS (expr)) + /* No calls/TARGET_EXPRs. */; + else + { + /* If we could initialize the reference directly from the call, it + wouldn't involve any copies. */ + STRIP_NOPS (expr); + if (TREE_CODE (expr) != CALL_EXPR + || !reference_related_p (non_reference (TREE_TYPE (expr)), type)) + return; + } + + auto_diagnostic_group d; + if (warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wrange_loop_construct, + "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 +12723,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 +12823,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 2091e0cd23b..b4df1834d04 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 @@ -3604,6 +3604,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 @@ -5273,6 +5291,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: 942ab9e9d4ff1da711daad3e8c71c57fd4c14035
