[PATCH] c++: Implement -Wrange-loop-construct [PR94695]

[Marek Polacek]

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