On 9/28/20 11:34 AM, Marek Polacek wrote:
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?
Not that I know of. It's just something I had in the back of my
mind.
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.
Sure, an optimization that changed code in a detectable way would
not be viable. But I wasn't thinking of actually changing the type
of the variable at this high level. What I meant is that it would
be nice to transform an example like this:
struct S { int i; char a[80]; };
const S a[] = { { 123, "abc" }, { 234, "bcd" }, { 345, "cde"} };
int f (const char *s)
{
for (auto x: a)
if (__builtin_strcmp (x.a, s) == 0)
return x.i;
return -1;
}
into this (when it's possible) and avoid issuing the warning:
int g (const char *s)
{
for (int i = 0; i != sizeof a / sizeof *a; ++i)
if (strcmp (a[i].a, s) == 0)
return a[i].i;
return -1;
}
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.
Yes, both look useful and I'm pretty sure at least the first one
has been requested.
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.
This is about proxies, like vector<bool>::reference, right?.
I'm also not sure that comes up enough to spend time on.
* -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? ;)
Thanks for all the detail!
I think the most valuable warnings are those that find either bugs
or likely mistakes that could cause the program to behave in
unintended ways. So of those above, -Wfor-loop-analysis warnings
seem the most useful to me.
A warning about expensive copies would also be useful (similar to
-Wsuggest-attribute=const or =pure). But since it's about
optimization and not about correctness, I'd expect it to only fire
when the optimizer cannot do a better job on its own without some
guidance/annotation from the programmer. GCC avoids issuing
-Wsuggest-attribute=const when it figures the constness of
a function on its own. Otherwise it feels like a recommendation
to work around shortcomings in the optimizer.
Martin
Marek
