Summary: | Eliminate WebKit2 compilation warnings. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Lukasz Slachciak <l.slachciak> | ||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, andersca, darin, mdwajahatali.siddiqui, rakuco, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Lukasz Slachciak
2011-07-22 05:50:04 PDT
Created attachment 101724 [details]
Compilator warnings patch
Looks OK to me. 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. Created attachment 107168 [details] more compiler warnings fix Addressing Comment#3 its WTF::notFound, also fixing some more warnings on webkit2 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 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. We normally want a separate bug for each patch. Putting two patches in a single bug is not good. (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. (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? (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. Created attachment 107329 [details]
compiler warnings fix updated
updated based on review
(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 (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 on attachment 107329 [details] compiler warnings fix updated Clearing flags on attachment: 107329 Committed r95154: <http://trac.webkit.org/changeset/95154> 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. Looks like this patch landed. If there are more bugs to fix, please open a new bug. |