Bug 10192 - UMBRELLA: WebKit should build with -Wshorten-64-to-32
Summary: UMBRELLA: WebKit should build with -Wshorten-64-to-32
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2006-08-01 14:02 PDT by Sam Weinig
Modified: 2014-03-19 14:07 PDT (History)
4 users (show)

See Also:


Attachments
patch (64.39 KB, patch)
2006-08-01 14:34 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
patch of trivial changes (51.02 KB, patch)
2006-08-02 14:51 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
patch of remaining less trivial changes (9.00 KB, patch)
2006-08-02 15:20 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
combind patch (56.22 KB, patch)
2006-08-03 11:33 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
remaining cases patch (7.59 KB, patch)
2006-08-04 15:30 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2006-08-01 14:02:06 PDT
Yet another gcc warning flag.
Comment 1 Sam Weinig 2006-08-01 14:34:00 PDT
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.
Comment 2 Darin Adler 2006-08-01 18:18:01 PDT
I'd rather first see a patch that has all non-debatable changes -- not including the typecasts that might be truncating values.
Comment 3 Sam Weinig 2006-08-02 14:51:24 PDT
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.
Comment 4 Darin Adler 2006-08-02 15:14:16 PDT
Comment on attachment 9834 [details]
patch of trivial changes

r=me
Comment 5 Sam Weinig 2006-08-02 15:20:10 PDT
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.
Comment 6 Darin Adler 2006-08-02 16:13:41 PDT
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.
Comment 7 Sam Weinig 2006-08-03 11:33:59 PDT
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.
Comment 8 Darin Adler 2006-08-03 19:35:20 PDT
Comment on attachment 9851 [details]
combind patch

r=me
Comment 9 Darin Adler 2006-08-03 19:36:34 PDT
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.
Comment 10 Sam Weinig 2006-08-04 08:53:08 PDT
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?
Comment 11 Darin Adler 2006-08-04 09:53:05 PDT
(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.
Comment 12 Sam Weinig 2006-08-04 11:57:15 PDT
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.
Comment 13 Sam Weinig 2006-08-04 15:30:16 PDT
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.
Comment 14 Eric Seidel (no email) 2007-10-01 08:51:25 PDT
Only WebKit remains with this issue:
// FIXME: <rdar://problem/5070292> WebKit should build with -Wshorten-64-to-32

Comment 15 Darin Adler 2007-10-01 09:04:08 PDT
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.
Comment 16 David Kilzer (:ddkilzer) 2013-02-14 10:43:26 PST
<rdar://problem/5070292>
Comment 17 David Kilzer (:ddkilzer) 2013-02-14 10:44:09 PST
On 32-bit today, there is only one issue left in WebHistory.mm.  Not sure about 64-bit.