On Fri, Sep 25, 2020 at 04:31:16PM -0600, Martin Sebor wrote:
> 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?
No, but perhaps we should add it. I don't know if we could still enable it by
-Wall. We'd have to handle guaranteed copy elision and also the case when we
pass classes by invisible reference. Unsure how much of the implementation
these warnings could share.
Do we have a request for the warning wrt passing chunky objects by value?
As a user, I'd probably want to have the option of figuring out where I'm
copying large types, since it can be a performance issue.
> 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?
Well, changing const auto x in
struct S { char arr[128]; S(); };
void
fn ()
{
S a[5];
for (const auto x : a)
decltype(x) k;
}
to const auto &x would break this code.
> 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 was a bit confusing to me too at first. It's about constructing expensive
copies in range-based for-loops. I don't think it's incidental that
it warns in loops only.
I'm not attached to the name but it's what Clang uses so we'll have to
follow.
> (It's impossible to tell from the Clang manual because its way
> of documenting warning options is to show examples of their text.)
Yes. I really like that we provide code snippets showing what a warning
is supposed to warn on in our manual. Let's keep it that way.
> 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).
I think right now it's like this (note the names and wording changed recently,
this is the latest version):
-> -Wfor-loop-analysis
|
-Wloop-analysis -| -> -Wrange-loop-bind-reference
| |
-> -Wrange-loop-analysis -|
|
-> -Wrange-loop-construct
* -Wloop-analysis and -Wrange-loop-analysis are umbrella flags.
* -Wfor-loop-analysis warns about
void f ()
{
for (int i = 0; i < 10;) {}
}
variable 'i' used in loop condition not modified in loop body
[-Wfor-loop-analysis]
and
void f ()
{
for (int i = 0; i < 10; i++)
{
i++;
}
}
warning: variable 'i' is incremented both in the loop header and in the loop
body [-Wfor-loop-analysis]
No plans to implement this warning now, but it'd probably be useful. Needs
to be implemented in both the C and C++ FE. Enabled by -Wall.
* -Wrange-loop-bind-reference warns about
void f ()
{
Container<int> C;
for (const int &x : C) {}
}
warning: loop variable 'x' binds to a temporary value produced by a range of
type 'Container<int>' [-Wrange-loop-bind-reference]
Not enabled by -Wall (anymore). Not sure if we should add this one.
* -Wrange-loop-construct warns about
struct S { char a[128]; };
void f ()
{
S arr[5];
for (const S x : arr) {}
}
warning: loop variable 'x' creates a copy from type 'const S'
[-Wrange-loop-construct]
and also about
void f ()
{
Container<int&> foo;
for (const double &x : foo) {}
}
warning: loop variable 'x' of type 'const double &' binds to a temporary
constructed from type 'int &' [-Wrange-loop-construct]
note: use non-reference type 'double' to make construction explicit or type
'const int &' to prevent copying
Both enabled by -Wall. My patch implements the first part. I think I should
also tackle the second part while at it.
Which one of these do you find most appealing? ;)
Marek
