c++: Implement -Wrange-loop-construct [PR94695]
authorMarek Polacek <polacek@redhat.com>
Thu, 24 Sep 2020 18:30:50 +0000 (14:30 -0400)
committerMarek Polacek <polacek@redhat.com>
Tue, 29 Sep 2020 23:03:04 +0000 (19:03 -0400)
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.  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
gcc/cp/call.c
gcc/cp/cp-tree.h
gcc/cp/parser.c
gcc/doc/invoke.texi
gcc/testsuite/g++.dg/warn/Wrange-loop-construct.C [new file with mode: 0644]

index 7761eefd2033f9becd5c7a954820731a64ded637..bbf7da896583450fd8c45899350cba4078df3468 100644 (file)
@@ -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.
index 5606389f4bd7e9c9293bffccf62766a733aa7b31..1e5fffe20ae09e16535081f082fe747b208c548b 100644 (file)
@@ -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
index a25934e3263934cb670b757e025f8e6f8a93170d..42d0d76bf2181fc296163636a6b98e0fdabc6fd5 100644 (file)
@@ -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> **,
index 8905833fbd6f2ff0d1e6cf93688557c2a7072970..cb4422764eda3b58a6b993f4f4e444443fde3211 100644 (file)
@@ -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 (loc, 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 to prevent copying");
+    }
+}
+
 /* 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;
 }
 
index 274c17ec17a874c2de8c6e982da1fa4999ff18fe..9a4903306d03b92932bf2d19404a75f7330cbefa 100644 (file)
@@ -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 (file)
index 0000000..3caf00d
--- /dev/null
@@ -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> ();
+}