On 9/25/20 6:01 PM, Marek Polacek wrote:
On Fri, Sep 25, 2020 at 04:09:44PM -0400, Jason Merrill via Gcc-patches wrote:
On 9/24/20 8:05 PM, Marek Polacek 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.
Would conv_binds_ref_to_prvalue (implicit_conversion (...)) do what you
want?
Yes, thanks. I played with conv_binds_ref_to_prvalue before to check
if the non-reference range-decl case creates a copy, but since the
types of the range decl and *__for_begin are the same, we only get
ck_identity for it. But I never tried it for the & case... Nevermind.
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) { }
| ^
| &
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.
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 | 13 ++
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, 309 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..860e4826057 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8429,6 +8429,19 @@ 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));
+ 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.
+}
+
/* 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,
+ "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.texi
index 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: c74e6f7cfd7a741fc0477fe3660eec57581b22c5
