Bug 175910 - Fix -Wcast-qual and -Wunused-lambda-capture warnings in WebCore with new clang compiler
Summary: Fix -Wcast-qual and -Wunused-lambda-capture warnings in WebCore with new clan...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-23 15:52 PDT by David Kilzer (:ddkilzer)
Modified: 2017-10-06 16:16 PDT (History)
17 users (show)

See Also:


Attachments
Patch (11.89 KB, patch)
2017-08-23 15:56 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Part 2 (v1) (11.19 KB, patch)
2017-08-25 16:45 PDT, David Kilzer (:ddkilzer)
ddkilzer: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (2.15 MB, application/zip)
2017-08-25 18:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1.98 MB, application/zip)
2017-08-25 18:32 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2017-08-23 15:52:37 PDT
Fix -Wcast-qual and -Wunused-lambda-capture warnings in WebCore with new clang compiler.

<rdar://problem/33667497>
Comment 1 David Kilzer (:ddkilzer) 2017-08-23 15:56:53 PDT
Created attachment 318933 [details]
Patch
Comment 2 Jiewen Tan 2017-08-23 16:09:30 PDT
Comment on attachment 318933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318933&action=review

> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:-139
> -        (uint8_t*)keyData.modulus().data(), keyData.modulus().size(),

In fact, I don't understand why (uint8_t*) is even needed initially.
Comment 3 David Kilzer (:ddkilzer) 2017-08-23 16:13:24 PDT
Comment on attachment 318933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318933&action=review

>> Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:-139
>> -        (uint8_t*)keyData.modulus().data(), keyData.modulus().size(),
> 
> In fact, I don't understand why (uint8_t*) is even needed initially.

Because data() returns a signed const char *, and const uint8_t* is the same as const unsigned char*.

In other words, it's a signed to unsigned type conversion.
Comment 4 WebKit Commit Bot 2017-08-24 11:35:43 PDT
Comment on attachment 318933 [details]
Patch

Clearing flags on attachment: 318933

Committed r221152: <http://trac.webkit.org/changeset/221152>
Comment 5 WebKit Commit Bot 2017-08-24 11:35:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 David Kilzer (:ddkilzer) 2017-08-24 16:10:09 PDT
Reopening because:

- Partial rollout in <https://trac.webkit.org/changeset/221159> to fix builds.
- Found a couple more items that need to be fixed (code movement from WebKit to WebCore).
Comment 7 David Kilzer (:ddkilzer) 2017-08-25 16:45:55 PDT
Created attachment 319117 [details]
Part 2 (v1)
Comment 8 youenn fablet 2017-08-25 17:34:55 PDT
Comment on attachment 319117 [details]
Part 2 (v1)

View in context: https://bugs.webkit.org/attachment.cgi?id=319117&action=review

> Source/WebCore/Modules/cache/WorkerCacheStorageConnection.cpp:130
> +    m_proxy.postTaskToLoader([protectedThis = makeRef(*this), requestIdentifier, cacheIdentifier](ScriptExecutionContext&) mutable {

It is true that capturing this and protectedThis is redundant.
Purpose was readability (which is debatable). There are probably other places in the code base that are doing the same thing.
Is it the idea to also change these other places?

Is the issue that protectedThis being unused is creating warnings?
That may be a bummer since there is a pattern to make use of this destroy-lambda-parameters after executing the lambda.
This can be used to destroy objects in a specific thread, or the destructor of the object might do a specific job, or we want to keep an object alive without needing to access it directly.
Or is clang smart enough to only check for parameters that are not owned by the lambda?
Comment 9 Build Bot 2017-08-25 18:15:54 PDT
Comment on attachment 319117 [details]
Part 2 (v1)

Attachment 319117 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4383643

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-delete.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-add.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-storage.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-storage-match.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-keys.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-matchAll.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html
Comment 10 Build Bot 2017-08-25 18:15:55 PDT
Created attachment 319125 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 11 Build Bot 2017-08-25 18:32:12 PDT
Comment on attachment 319117 [details]
Part 2 (v1)

Attachment 319117 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4383646

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-delete.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-add.https.html
http/wpt/cache-storage/cache-put-keys.https.any.worker.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-storage.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-storage-match.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-keys.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-matchAll.https.html
imported/w3c/web-platform-tests/service-workers/cache-storage/worker/cache-match.https.html
Comment 12 Build Bot 2017-08-25 18:32:14 PDT
Created attachment 319126 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 13 David Kilzer (:ddkilzer) 2017-10-02 16:48:28 PDT
Let's just close this one, and open a new bug for the remaining patch.  It will be less confusing that way.