WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
137565
Avoid defaulting to capture-by-value for C++11 lambdas in WebCore
https://bugs.webkit.org/show_bug.cgi?id=137565
Summary
Avoid defaulting to capture-by-value for C++11 lambdas in WebCore
Zan Dobersek
Reported
2014-10-09 13:12:49 PDT
Using the default capture-by-value should be avoided as it can produce various negative side effects, e.g. capturing the this pointer when a member variable is used inside the lambda, and thus failing to make the lambda object self-contained. It also requires the author to explicitly list the variables she intends to use inside the lambda, providing one additional checkpoint at ensuring that the correct variables are used.
Attachments
Patch
(40.48 KB, patch)
2014-10-09 13:25 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2014-10-09 13:25:36 PDT
Created
attachment 239563
[details]
Patch
WebKit Commit Bot
Comment 2
2014-10-09 13:28:10 PDT
Attachment 239563
[details]
did not pass style-queue: ERROR: Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.cpp:227: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:333: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:375: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp:86: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/WebCore/Modules/webdatabase/SQLCallbackWrapper.h:78: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 5 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Zan Dobersek
Comment 3
2014-10-09 13:28:37 PDT
Comment on
attachment 239563
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=239563&action=review
Outlined a few problems that could have been avoided, most likely of my doing.
> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:422 > - m_loaderProxy.postTaskToLoader([=](ScriptExecutionContext& context) { > + m_loaderProxy.postTaskToLoader([peer, urlCopy, protocolCopy] (ScriptExecutionContext& context) { > ASSERT(isMainThread()); > ASSERT_UNUSED(context, context.isDocument()); > ASSERT(peer); > > - peer->connect(url, protocol); > + peer->connect(urlCopy, protocolCopy);
Case in point.
> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:439 > - m_loaderProxy.postTaskToLoader([=](ScriptExecutionContext& context) { > + m_loaderProxy.postTaskToLoader([peer, messageCopy] (ScriptExecutionContext& context) { > ASSERT(isMainThread()); > ASSERT_UNUSED(context, context.isDocument()); > ASSERT(peer); > > - peer->send(message); > + peer->send(messageCopy);
Ditto.
> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:558 > - m_loaderProxy.postTaskToLoader([=] (ScriptExecutionContext& context) { > + m_loaderProxy.postTaskToLoader([peer, reasonCopy] (ScriptExecutionContext& context) { > ASSERT(isMainThread()); > ASSERT_UNUSED(context, context.isDocument()); > ASSERT(peer); > > - peer->fail(reason); > + peer->fail(reasonCopy);
Ditto.
> Source/WebCore/dom/StringCallback.cpp:43 > RefPtr<StringCallback> protector(this); > - context->postTask([=] (ScriptExecutionContext&) { > - this->handleEvent(data); > + context->postTask([protector, data] (ScriptExecutionContext&) { > + protector->handleEvent(data);
Ditto.
Darin Adler
Comment 4
2014-10-09 14:52:26 PDT
Comment on
attachment 239563
[details]
Patch I like this better. I remember being a little puzzled by the [=] syntax earlier, since I have always used the exhaustive list of what to capture.
Zan Dobersek
Comment 5
2014-10-10 03:28:43 PDT
Comment on
attachment 239563
[details]
Patch Clearing flags on attachment: 239563 Committed
r174581
: <
http://trac.webkit.org/changeset/174581
>
Zan Dobersek
Comment 6
2014-10-10 03:28:52 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug