Bug 10192 – UMBRELLA: WebKit should build with -Wshorten-64-to-32

• Status: NEW
• Component: WebCore Misc.

[Sam Weinig]

Yet another gcc warning flag.

[Sam Weinig]

Created attachment 9802 [details]
patch

Attaching patch with ChangeLogs.  All of the changes to the code are making explicit, already implicit conversions/casts from 64 bit types (such as double or long long) to 32 bit types.  Some of these conversions are worrisome as data is potentially being lost.

[Darin Adler]

I'd rather first see a patch that has all non-debatable changes -- not including the typecasts that might be truncating values.

[Sam Weinig]

Created attachment 9834 [details]
patch of trivial changes

This patch includes only the trivial changes needed to compile with -Wshorten-64-to-32.  The changes include adding 'f's to the end of float literals and switching some cos()'s and ceil()'s to cosf()'s and ceilf()'s.  I've also added the flag to the Xcode project for JaveScriptGlue as it needs no modifications to compile with it.

[Darin Adler]

Comment on attachment 9834 [details]
patch of trivial changes

r=me

[Sam Weinig]

Created attachment 9837 [details]
patch of remaining less trivial changes

This third patch contains the remaining changes.  NOTE: none of the changes in this patch change any behavior, they simply explicitly down cast 64 bit types to 32 bit types that would be implicitly down cast anyway.  The question is if those implicit conversions were safe to begin with and if not how we should deal with them.

[Darin Adler]

Comment on attachment 9837 [details]
patch of remaining less trivial changes

The ones in JSNodeList are OK, because that's just test code. So we can definitely land the JavaScriptCore part of this.

The one in WebFrameBridge.m looks right, so we can do the cast there.

The one in WebNodeHighlightView.m looks fine.

I'm not sure about the rest.

[Sam Weinig]

Created attachment 9851 [details]
combind patch

This patch includes all the trivial changes and the changes approved by Darin.  Because some of the changes in WebKit are still under debate, I cannot turn on the -Wshorten-64-to-32 warning for it.  The warning is on for WebCore, JavaScriptCore, and JavaScriptGlue though.

[Darin Adler]

Comment on attachment 9851 [details]
combind patch

r=me

[Darin Adler]

Comment on attachment 9837 [details]
patch of remaining less trivial changes

Clearing the review flag on this for now. We should go over the other cases in WebKit and see what we can do for each one. Either there's some reason we can guarantee the value fits in 32 bits or we need some checks and error handling.

[Sam Weinig]

Darin, 
What do you suggest I do in the mean time.  Should I land the combined patch as is and open a new bug for the remaining cases, or should I wait until the rest is worked out?

[Darin Adler]

(In reply to comment #10)
> What do you suggest I do in the mean time.  Should I land the combined patch as
> is and open a new bug for the remaining cases, or should I wait until the rest
> is worked out?

You should definitely land the combined patch, and then we should work on the remaining cases. I'm not sure we need a new bug report, though. I'd be OK with landing the combined patch, clearing the review flag, and leaving this one open.

[Sam Weinig]

Comment on attachment 9851 [details]
combind patch

Landed the combined patch in r15799. I am clearing the review flag so we can continue to use this bug report.

[Sam Weinig]

Created attachment 9882 [details]
remaining cases patch

I'm posting what a patch with the code necessary to get WebKit to compile with the flag, but it is not intended for real use, only to highlight the remaining cases.

[Eric Seidel (no email)]

Only WebKit remains with this issue:
// FIXME: <rdar://problem/5070292> WebKit should build with -Wshorten-64-to-32

[Darin Adler]

But:

    1) Newer versions of gcc warn a lot more with this warning and we had to turn it off in some places because of that.

and

    2) Oliver Hunt found that the feature branch had a lot of float/double conversions that run afoul of this, so when we merge the feature branch we will "backslide" a little on this warning.

[David Kilzer (:ddkilzer)]

<rdar://problem/5070292>

[David Kilzer (:ddkilzer)]

On 32-bit today, there is only one issue left in WebHistory.mm.  Not sure about 64-bit.