Summary: | [WK2] Fix unused parameter compile warning | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Miyoung Shin <myid.shin> | ||||||||||||
Component: | WebKit2 | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, gyuyoung.kim, ossy, thiago.lacerda | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Other | ||||||||||||||
OS: | Linux | ||||||||||||||
Attachments: |
|
Description
Miyoung Shin
2014-04-04 21:44:36 PDT
Created attachment 228663 [details]
Patch
Comment on attachment 228663 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228663&action=review > Source/WebKit2/ChangeLog:3 > + [EFL][WK2] Fix unused parameter compile warning. This patch is not only for [EFL]. Please remove [EFL] prefix. > Source/WebKit2/UIProcess/API/APIDownloadClient.h:51 > + virtual void didReceiveData(WebKit::WebContext*, WebKit::DownloadProxy*, uint64_t) { } I prefer to use UNUSED_PARAM when parameter name is critical for clarity. In this case, length is that case. UNUSED_PARAM(length); ? Besides there was a discussion how to handle unused parameter in below thread. https://lists.webkit.org/pipermail/webkit-dev/2012-October/022369.html > Source/WebKit2/UIProcess/API/APIDownloadClient.h:52 > + virtual bool shouldDecodeSourceDataOfMIMEType(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&) { return true; } ditto. > Source/WebKit2/UIProcess/API/APIDownloadClient.h:53 > + virtual WTF::String decideDestinationWithSuggestedFilename(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&, bool&) { return { }; } ditto. > Source/WebKit2/UIProcess/API/APIDownloadClient.h:54 > + virtual void didCreateDestination(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&) { } ditto. Created attachment 228669 [details]
Patch
Comment on attachment 228669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228669&action=review This patch modifies WK2 common part though, this is a trivial build warning fix related to unused param. This change is based on below discussion https://lists.webkit.org/pipermail/webkit-dev/2012-October/022369.html LGTM. > Source/WebKit2/ChangeLog:7 > + Please add description before landing. Created attachment 228671 [details]
Patch for landing
Comment on attachment 228671 [details] Patch for landing Clearing flags on attachment: 228671 Committed r166848: <http://trac.webkit.org/changeset/166848> All reviewed patches have been landed. Closing bug. Comment on attachment 228671 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=228671&action=review > Source/WebKit2/UIProcess/API/APIDownloadClient.h:63 > + virtual void didReceiveData(WebKit::WebContext*, WebKit::DownloadProxy*, uint64_t length) { UNUSED_PARAM(length); } > + virtual bool shouldDecodeSourceDataOfMIMEType(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String& mimeType) > + { > + UNUSED_PARAM(mimeType); > + return true; > + } > + virtual WTF::String decideDestinationWithSuggestedFilename(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String& filename, bool& allowOverwrite) > + { > + UNUSED_PARAM(filename); > + UNUSED_PARAM(allowOverwrite); > + return { }; > + } > + virtual void didCreateDestination(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String& path) { UNUSED_PARAM(path); } This is not what we want. We should just remove the argument names, not use UNUSED_PARAM and not reformat the code. Comment on attachment 228671 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=228671&action=review >> Source/WebKit2/UIProcess/API/APIDownloadClient.h:63 >> + virtual void didCreateDestination(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String& path) { UNUSED_PARAM(path); } > > This is not what we want. We should just remove the argument names, not use UNUSED_PARAM and not reformat the code. APIFindClient.h, APIHistoryClient.h, APILoaderClient.h and APIUIClient.h have already used this style. Do you think we should remove the arguments as well ? Reopening to attach new patch. Created attachment 228717 [details]
Patch
(In reply to comment #11) > Created an attachment (id=228717) [details] > Patch Darin, do you think this one is fine ? I agree with Darin, we shouldn't add UNUSED_PARAM everywhere. We should simple remove argument names if it is possible. But sometimes a parameter is used inside an #if PLATFORM(FOO) guard, in these cases we can use the UNUSED_PARAM macro in the #else case. cc Thiago and Benajmin too - author and reviewer of r165794 which added many UNUSED_PARAM to these headers to revise that patch too. (In reply to comment #13) > I agree with Darin, we shouldn't add UNUSED_PARAM everywhere. We should > simple remove argument names if it is possible. But sometimes a parameter > is used inside an #if PLATFORM(FOO) guard, in these cases we can use the > UNUSED_PARAM macro in the #else case. > > cc Thiago and Benajmin too - author and reviewer of r165794 which > added many UNUSED_PARAM to these headers to revise that patch too. There was already a rule to handle UNUSED_PARAM(). https://lists.webkit.org/pipermail/webkit-dev/2012-October/022369.html In this case, I thought primitive type param is better to use UNUSED_PARAM() to clarify the parameter role. Besides r165794 also used it. However, it is better not to use UNUSED_PARAM in this case, I will follow it. Attachment 228717 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:54: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:58: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:62: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Total errors found: 3 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 228717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=228717&action=review The patch looks good to me, but I let Darin to do the final review. Additionally we can remove UNUSED_PARAM macros from APIUIClient.h too. > Source/WebKit2/UIProcess/API/APIDownloadClient.h:55 > + virtual bool shouldDecodeSourceDataOfMIMEType(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&) > + { return true; } > + virtual WTF::String decideDestinationWithSuggestedFilename(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&, bool&) > + { return { }; } I think we can avoid these linebreaks. Created attachment 228725 [details]
Patch
(In reply to comment #16) > (From update of attachment 228717 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=228717&action=review > > The patch looks good to me, but I let Darin to do the final review. > Additionally we can remove UNUSED_PARAM macros from APIUIClient.h too. Fixed. > > Source/WebKit2/UIProcess/API/APIDownloadClient.h:55 > > + virtual bool shouldDecodeSourceDataOfMIMEType(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&) > > + { return true; } > > + virtual WTF::String decideDestinationWithSuggestedFilename(WebKit::WebContext*, WebKit::DownloadProxy*, const WTF::String&, bool&) > > + { return { }; } > > I think we can avoid these linebreaks. Fixed. Thanks. Attachment 228725 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:54: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:58: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
ERROR: Source/WebKit2/UIProcess/API/APIPolicyClient.h:62: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5]
Total errors found: 3 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 228725 [details] Patch Clearing flags on attachment: 228725 Committed r166914: <http://trac.webkit.org/changeset/166914> All reviewed patches have been landed. Closing bug. |