Cleaning up implicit `this` captures

[Devon Loehr]

We currently disable clang's warning about lambdas which implicitly capture `this` by value

(https://crbug.com/40255410), which can potentially produce unintuitive behavior (as discussed here: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0806r2.html).

I'd like to re-enable this warning, which entails modifying each instance to make the capture explicit, e.g. changing `[=]` to `[=, this]`. Since this is just removing syntactic sugar, there should be no changes to the compiled code. Once we've cleaned up all the uses, we can re-enable the warning to ensure no further ones appear.

On my windows computer, I count 305 instances in 54 files, across a variety of projects, some of which are third-party. Automatically doing the replacement via a script targeting the warning sites isn't too hard; I expect the main issue will be doing so in each of the different projects. The internal projects affected are v8, swiftshader, and dawn. The third-party projects are marl, dxc, and eigen3.

Does anyone have objections or thoughts about this?

--Devon Loehr

[Daniel Cheng]

+1 for doing this

I think v8 and dawn both use Chrome's build directory, right? So fixing the warnings there and preventing backsliding shouldn't be too hard, since we can just enable a GN arg to enable the warning in those projects.

I don't think Swiftshader is actively developed anymore, so there shouldn't be any backsliding there.

Daniel

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/2be8f92e-5f90-4f32-929c-7ce57154a9b9n%40chromium.org.

[Joe Mason]

I commonly use `[&]` in unit tests to just capture the entire environment in BindLambdaForTesting, especially since TEST_F is secretly a method so `this` must be captured to call test fixture methods. Having to use `[&, this]` would be awkward and less readable IMHO. Is there any way to exempt *_unittest.cc files, etc, from the warning?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKoRq2RxipheVbTLUopzpwvwxP7HueJK8gcKCmA9MZqd-A%40mail.gmail.com.

[danakj]

On Wed, Jul 10, 2024 at 11:33 AM 'Joe Mason' via cxx <c...@chromium.org> wrote:

I commonly use `[&]` in unit tests to just capture the entire environment in BindLambdaForTesting, especially since TEST_F is secretly a method so `this` must be captured to call test fixture methods. Having to use `[&, this]` would be awkward and less readable IMHO. Is there any way to exempt *_unittest.cc files, etc, from the warning?

The rule only applies to [=] (copy everything): "The implicit capture of *this when the capture default is = is deprecated.(since C++20)"

 

On Tue, Jul 9, 2024 at 4:45 PM Daniel Cheng <dch...@chromium.org> wrote:

+1 for doing this

I think v8 and dawn both use Chrome's build directory, right? So fixing the warnings there and preventing backsliding shouldn't be too hard, since we can just enable a GN arg to enable the warning in those projects.

I don't think Swiftshader is actively developed anymore, so there shouldn't be any backsliding there.

Daniel

On Tue, 9 Jul 2024 at 13:32, 'Devon Loehr' via cxx <c...@chromium.org> wrote:

We currently disable clang's warning about lambdas which implicitly capture `this` by value

(https://crbug.com/40255410), which can potentially produce unintuitive behavior (as discussed here: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0806r2.html).

I'd like to re-enable this warning, which entails modifying each instance to make the capture explicit, e.g. changing `[=]` to `[=, this]`. Since this is just removing syntactic sugar, there should be no changes to the compiled code. Once we've cleaned up all the uses, we can re-enable the warning to ensure no further ones appear.

On my windows computer, I count 305 instances in 54 files, across a variety of projects, some of which are third-party. Automatically doing the replacement via a script targeting the warning sites isn't too hard; I expect the main issue will be doing so in each of the different projects. The internal projects affected are v8, swiftshader, and dawn. The third-party projects are marl, dxc, and eigen3.

Does anyone have objections or thoughts about this?

--Devon Loehr

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/2be8f92e-5f90-4f32-929c-7ce57154a9b9n%40chromium.org.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKoRq2RxipheVbTLUopzpwvwxP7HueJK8gcKCmA9MZqd-A%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAH%3DT95RoeDHLnGve7inmEkub2FQBwqC9HKXyAbz%2BHhQrwosvvw%40mail.gmail.com.

[Joe Mason]

On Wed, Jul 10, 2024 at 11:44 AM danakj <dan...@chromium.org> wrote:

On Wed, Jul 10, 2024 at 11:33 AM 'Joe Mason' via cxx <c...@chromium.org> wrote:

I commonly use `[&]` in unit tests to just capture the entire environment in BindLambdaForTesting, especially since TEST_F is secretly a method so `this` must be captured to call test fixture methods. Having to use `[&, this]` would be awkward and less readable IMHO. Is there any way to exempt *_unittest.cc files, etc, from the warning?

The rule only applies to [=] (copy everything): "The implicit capture of *this when the capture default is = is deprecated.(since C++20)"

+1 from me, then!

[Avi Drissman]

It seems to me that you are understating things. It's not just that the behavior is unintuitive (which it is), it's that P0806R2, designating that behavior as explicitly deprecated, was accepted into C++20. Given that the disabled warning is trying to get us to stop doing something deprecated, I'm in favor of your proposal.

Avi

--