Bug 65025 - Eliminate WebKit2 compilation warnings.
Summary: Eliminate WebKit2 compilation warnings.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-22 05:50 PDT by Lukasz Slachciak
Modified: 2011-10-25 20:01 PDT (History)
6 users (show)

See Also:


Attachments
Compilator warnings patch (2.26 KB, patch)
2011-07-22 06:39 PDT, Lukasz Slachciak
darin: review+
Details | Formatted Diff | Diff
more compiler warnings fix (3.66 KB, patch)
2011-09-13 05:56 PDT, Wajahat Siddiqui
no flags Details | Formatted Diff | Diff
compiler warnings fix updated (2.09 KB, patch)
2011-09-14 07:06 PDT, Wajahat Siddiqui
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lukasz Slachciak 2011-07-22 05:50:04 PDT
Eliminate compilator warnings for WebKit2 related to wrong type comparison.
Comment 1 Lukasz Slachciak 2011-07-22 06:39:57 PDT
Created attachment 101724 [details]
Compilator warnings patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-07-25 09:03:07 PDT
Looks OK to me.
Comment 3 Alexis Menard (darktears) 2011-08-01 11:36:15 PDT
Comment on attachment 101724 [details]
Compilator warnings patch

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

LGTM.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1268
> +        if (wmodeIndex == notFound) {

I guess it is using the WTF::NotFound.
Comment 4 Wajahat Siddiqui 2011-09-13 05:56:57 PDT
Created attachment 107168 [details]
more compiler warnings fix

Addressing Comment#3 its WTF::notFound, also fixing some more warnings on webkit2
Comment 5 Darin Adler 2011-09-13 09:49:12 PDT
Comment on attachment 107168 [details]
more compiler warnings fix

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

> Source/WebKit2/UIProcess/WebContext.cpp:324
> +        String sampleLogFilePath = String::format("WebProcess%llu", static_cast<long long unsigned int>(now));

The name we use for this type in WebKit is "unsigned long long" rather than "long long unsigned int". Anders may have additional comments. I know he often uses uint64_t intentionally so we might instead want to use the string that lets us format a uint64_t instead of changing the typecast.

> Source/WebKit2/UIProcess/WebContext.cpp:733
> +    String sampleLogFilePath = String::format("WebProcess%llu", static_cast<long long unsigned int>(now));

Ditto.

> Source/WebKit2/ChangeLog:3
> +        Eliminate WebKit2 compilation warnings.

It would be better if this said what compiler and platform these warnings were found with.
Comment 6 Darin Adler 2011-09-13 09:49:41 PDT
Comment on attachment 107168 [details]
more compiler warnings fix

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

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1269
> +        if (wmodeIndex == WTF::notFound) {

We normally just call this notFound. I am surprised that the explicit WTF:: is needed, and if it’s not needed, please omit it.
Comment 7 Darin Adler 2011-09-13 09:50:00 PDT
We normally want a separate bug for each patch. Putting two patches in a single bug is not good.
Comment 8 Anders Carlsson 2011-09-13 10:00:22 PDT
(In reply to comment #5)
> (From update of attachment 107168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107168&action=review
> 
> > Source/WebKit2/UIProcess/WebContext.cpp:324
> > +        String sampleLogFilePath = String::format("WebProcess%llu", static_cast<long long unsigned int>(now));
> 
> The name we use for this type in WebKit is "unsigned long long" rather than "long long unsigned int". Anders may have additional comments. I know he often uses uint64_t intentionally so we might instead want to use the string that lets us format a uint64_t instead of changing the typecast.

We only use explicitly sized types when dealing with CoreIPC messages; you want to avoid sending an unsigned long between a 64-bit and a 32-bit process.
Comment 9 Darin Adler 2011-09-13 10:26:07 PDT
(In reply to comment #8)
> (In reply to comment #5)
> > (From update of attachment 107168 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=107168&action=review
> > 
> > > Source/WebKit2/UIProcess/WebContext.cpp:324
> > > +        String sampleLogFilePath = String::format("WebProcess%llu", static_cast<long long unsigned int>(now));
> > 
> > The name we use for this type in WebKit is "unsigned long long" rather than "long long unsigned int". Anders may have additional comments. I know he often uses uint64_t intentionally so we might instead want to use the string that lets us format a uint64_t instead of changing the typecast.
> 
> We only use explicitly sized types when dealing with CoreIPC messages; you want to avoid sending an unsigned long between a 64-bit and a 32-bit process.

Sure, but this is about interoperating with printf format strings. The two ways to do that are to use the macros for creating the format strings, or casting to a built-in language type such as "unsigned long long". Do you have a comment on which is preferred? Maybe the right fix is to use a format string more like "%.0f" instead of trying to cast to an integral type?
Comment 10 Anders Carlsson 2011-09-13 10:31:05 PDT
(In reply to comment #9)
> Sure, but this is about interoperating with printf format strings. The two ways to do that are to use the macros for creating the format strings, or casting to a built-in language type such as "unsigned long long". Do you have a comment on which is preferred? Maybe the right fix is to use a format string more like "%.0f" instead of trying to cast to an integral type?

I don't have a strong opinion. In this particular case I think a full date + time string would be better, but that might be outside the scope of this patch.
Comment 11 Wajahat Siddiqui 2011-09-14 07:06:31 PDT
Created attachment 107329 [details]
compiler warnings fix updated

updated based on review
Comment 12 Wajahat Siddiqui 2011-09-14 07:12:20 PDT
(In reply to comment #5)
Thanks for the review !!

> (From update of attachment 107168 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107168&action=review
> 
> > Source/WebKit2/UIProcess/WebContext.cpp:324
> > +        String sampleLogFilePath = String::format("WebProcess%llu", static_cast<long long unsigned int>(now));
> 
> The name we use for this type in WebKit is "unsigned long long" rather than "long long unsigned int". Anders may have additional comments. I know he often uses uint64_t intentionally so we might instead want to use the string that lets us format a uint64_t instead of changing the typecast.

Based on below comments its better to ignore fixing this for now as i see a need to capture full date + time and not floating values of time. Since fixing this will involve changing string format will submit as separate patch.
Hence reverting the changes :) 

> 
> > Source/WebKit2/UIProcess/WebContext.cpp:733
> > +    String sampleLogFilePath = String::format("WebProcess%llu", static_cast<long long unsigned int>(now));
> 
> Ditto.

Same as above

> 
> > Source/WebKit2/ChangeLog:3
> > +        Eliminate WebKit2 compilation warnings.
> 
> It would be better if this said what compiler and platform these warnings were found with.

Done
Comment 13 Wajahat Siddiqui 2011-09-14 07:25:06 PDT
(In reply to comment #7)
> We normally want a separate bug for each patch. Putting two patches in a single bug is not good.

Correct, the older patch is obsolete.
Comment 14 WebKit Review Bot 2011-09-14 19:52:20 PDT
Comment on attachment 107329 [details]
compiler warnings fix updated

Clearing flags on attachment: 107329

Committed r95154: <http://trac.webkit.org/changeset/95154>
Comment 15 Adam Barth 2011-10-15 01:49:50 PDT
Comment on attachment 107168 [details]
more compiler warnings fix

Cleared Darin Adler's review+ from obsolete attachment 107168 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 16 Adam Barth 2011-10-25 20:01:09 PDT
Looks like this patch landed.  If there are more bugs to fix, please open a new bug.