Re: [PATCH v3] c++: Implement -Wrange-loop-construct [PR94695]
On Mon, Sep 28, 2020 at 03:05:55PM -0400, Jason Merrill via Gcc-patches wrote:
> 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 " 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?
Yeah, I wish there was a way to avoid it. But I don't think we have
a TREE_TYPE bit that says that a type was deduced from auto/decltype(auto).
I'll just avoid printing the type...
> > + auto_diagnostic_group d;
> > + if (warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wrange_loop_construct,
>
> Why not use 'loc' here?
Fixed.
> OK however you want to resolve these comments.
Pushed, thanks.
Marek
Re: [PATCH v3] c++: Implement -Wrange-loop-construct [PR94695]
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 " 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 (_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
Re: [PATCH v3] c++: Implement -Wrange-loop-construct [PR94695]
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 " 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| 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 (_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
