Fix unused lambda captures. (issue 2645613003 by krasin@chromium.org)

211 views
Skip to first unread message

kra...@chromium.org

unread,
Jan 18, 2017, 1:46:35 PM1/18/17
to esp...@chromium.org, chromium...@chromium.org, ajuma+wat...@chromium.org, dongseo...@intel.com, ju...@chromium.org, har...@chromium.org, caba...@adobe.com, blink-...@chromium.org
Reviewers: esprehn
CL: https://codereview.chromium.org/2645613003/

Message:
Hi Elliott!

please, review this small pack of fixes to the unused lambda captures. We need
to do this cleanup in order to adapt to the new (more strict) version of Clang.

Generally, not having unused <imports/includes/arguments/lamda captures> is good
anyway.

Description:
Fix unused lambda captures.

Clang just got more strict about unused lambda captures,
and that requires us to clean all places with this issue
across all the Chromium code base. This CL fixes all such
cases in WebKit/Blink.

BUG=681912

Affected files (+4, -4 lines):
M third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
M third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp


Index: third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
diff --git a/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp b/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
index cda9d72b40d949ba8c4524480a519dc7bbc7d82a..6d9f4a3e0bdd50b7fc6f5caae5f49e9ed7050cc9 100644
--- a/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
+++ b/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
@@ -622,7 +622,7 @@ void BaseRenderingContext2D::drawPathInternal(
if (!drawingCanvas())
return;

- if (draw([&skPath, this](SkCanvas* c, const SkPaint* paint) // draw lambda
+ if (draw([&skPath](SkCanvas* c, const SkPaint* paint) // draw lambda
{ c->drawPath(skPath, *paint); },
[](const SkIRect& rect) // overdraw test lambda
{ return false; },
@@ -682,7 +682,7 @@ void BaseRenderingContext2D::fillRect(double x,
return;

SkRect rect = SkRect::MakeXYWH(x, y, width, height);
- draw([&rect, this](SkCanvas* c, const SkPaint* paint) // draw lambda
+ draw([&rect](SkCanvas* c, const SkPaint* paint) // draw lambda
{ c->drawRect(rect, *paint); },
[&rect, this](const SkIRect& clipBounds) // overdraw test lambda
{ return rectContainsTransformedRect(rect, clipBounds); },
@@ -719,7 +719,7 @@ void BaseRenderingContext2D::strokeRect(double x,
SkRect rect = SkRect::MakeXYWH(x, y, width, height);
FloatRect bounds = rect;
inflateStrokeRect(bounds);
- draw([&rect, this](SkCanvas* c, const SkPaint* paint) // draw lambda
+ draw([&rect](SkCanvas* c, const SkPaint* paint) // draw lambda
{ strokeRectOnCanvas(rect, c, paint); },
[](const SkIRect& clipBounds) // overdraw test lambda
{ return false; },
Index: third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
diff --git a/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp b/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
index 080321b78f4b28f0a2fc8a443ff03f1690615ec6..2546e931500e2a4ca1ec38a699c8e994a770d682 100644
--- a/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
+++ b/third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
@@ -903,7 +903,7 @@ void CanvasRenderingContext2D::drawTextInternal(
}

draw(
- [&font, this, &textRunPaintInfo, &location](
+ [&font, &textRunPaintInfo, &location](
SkCanvas* c, const SkPaint* paint) // draw lambda
{
font.drawBidiText(c, textRunPaintInfo, location,


esp...@chromium.org

unread,
Jan 18, 2017, 2:57:53 PM1/18/17
to kra...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dongseo...@intel.com, har...@chromium.org, ju...@chromium.org
Does capturing |this| have any side effects? Ex. You need to do it to call any
instance methods?

Lgtm

https://codereview.chromium.org/2645613003/

kra...@chromium.org

unread,
Jan 18, 2017, 3:40:46 PM1/18/17
to esp...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dongseo...@intel.com, har...@chromium.org, ju...@chromium.org
On 2017/01/18 19:57:52, esprehn wrote:
> Does capturing |this| have any side effects? Ex. You need to do it to call any
> instance methods?
>
> Lgtm

Correct. If an instance method is accessed, the compiler will force to capture
this.
Consider the following program:

#include <stdio.h>

class Lala {
public:
void Do() {
auto g = []() { return Print(); } ;
g();
}

void Print() {
printf("hello!\n");
}

};

int main(void) {
Lala lala;
lala.Do();
}

In this case, Do creates a lambda that accesses an instance method Print, but it
does not capture |this|. Clang throws the following error:

$ clang++ lala.cc -o lala -std=c++11 && ./lala
lala.cc:6:28: error: 'this' cannot be implicitly captured in this context
auto g = []() { return Print(); } ;
^
1 error generated.

After changing the line to

auto g = [this]() { return Print(); } ;

the program compiles and runs:

$ clang++ lala.cc -o lala -std=c++11 && ./lala
hello!

Now, if we make Print static, it will complain about the capture of |this| being
unused:

$ clang++ lala.cc -o lala -std=c++11 -Wall -Werror && ./lala
lala.cc:6:15: error: lambda capture 'this' is not used
[-Werror,-Wunused-lambda-capture]
auto g = [this]() { return Print(); } ;
^
1 error generated.

The cases fixed in the current CL fall in the latter case: they don't use this
by any means.

https://codereview.chromium.org/2645613003/

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 18, 2017, 3:41:34 PM1/18/17
to kra...@chromium.org, esp...@chromium.org, commi...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dongseo...@intel.com, har...@chromium.org, ju...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Jan 18, 2017, 4:06:53 PM1/18/17
to kra...@chromium.org, esp...@chromium.org, commi...@chromium.org, ajuma+wat...@chromium.org, blink-...@chromium.org, caba...@adobe.com, chromium...@chromium.org, dongseo...@intel.com, har...@chromium.org, ju...@chromium.org
Reply all
Reply to author
Forward
0 new messages