Bug 65025 – Eliminate WebKit2 compilation warnings.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Lukasz Slachciak]

Eliminate compilator warnings for WebKit2 related to wrong type comparison.

[Lukasz Slachciak]

Created attachment 101724 [details]
Compilator warnings patch

[Raphael Kubo da Costa (:rakuco)]

Looks OK to me.

[Alexis Menard (darktears)]

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.

[Wajahat Siddiqui]

Created attachment 107168 [details]
more compiler warnings fix

Addressing Comment#3 its WTF::notFound, also fixing some more warnings on webkit2

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

We normally want a separate bug for each patch. Putting two patches in a single bug is not good.

[Anders Carlsson]

(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.

[Darin Adler]

(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?

[Anders Carlsson]

(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.

[Wajahat Siddiqui]

Created attachment 107329 [details]
compiler warnings fix updated

updated based on review

[Wajahat Siddiqui]

(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

[Wajahat Siddiqui]

(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.

[WebKit Review Bot]

Comment on attachment 107329 [details]
compiler warnings fix updated

Clearing flags on attachment: 107329

Committed r95154: <http://trac.webkit.org/changeset/95154>

[Adam Barth]

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.

[Adam Barth]

Looks like this patch landed.  If there are more bugs to fix, please open a new bug.