Bug 11508 - Undisable some warnings for JSImmediate.h
Summary: Undisable some warnings for JSImmediate.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: PC Windows XP
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks: 11504
  Show dependency treegraph
 
Reported: 2006-11-03 16:26 PST by Don Gibson
Modified: 2006-11-11 01:00 PST (History)
1 user (show)

See Also:


Attachments
patch v1 (5.39 KB, patch)
2006-11-03 18:16 PST, Don Gibson
ap: review-
Details | Formatted Diff | Diff
proposed patch (10.52 KB, patch)
2006-11-09 10:50 PST, Alexey Proskuryakov
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Gibson 2006-11-03 16:26:21 PST
The patch for bug 11504 purported to fix compile warnings on 32-bit platforms, but actually broke the Windows build for 32-bit.  (The Windows build was already broken for lots of other reasons, but I'm working on those, which is how I noticed this.)  At least with MSVC, you can't just stick a 64-bit value in an enum.

Coming shortly: a patch that both fixes this so the build works again, and finishes off the work of that patch by making the toDouble()/fromDouble() functions also check 32- and 64-bit mode at compile time rather than runtime.
Comment 1 Don Gibson 2006-11-03 18:16:29 PST
Created attachment 11369 [details]
patch v1

It's a bit hard for reviewers to test that this fixes the Windows build without the other fixes I have (not yet posted publicly) that do that, but people can certainly review whether the approach is appropriate.
Comment 2 Alexey Proskuryakov 2006-11-04 00:36:05 PST
Comment on attachment 11369 [details]
patch v1

Looks good to me, but r- for style issues - pointer types should be written with no space between the type name and the *.

Perhaps it would make sense to merge all the templates into one (FloatingPointBitValues or something).
Comment 3 Alexey Proskuryakov 2006-11-04 05:43:49 PST
Also, this breaks Mac build, because gcc doesn't like template specialization inside classes. I'll look into fixing this.
Comment 4 Alexey Proskuryakov 2006-11-04 08:52:14 PST
(In reply to comment #0)
> The patch for bug 11504 purported to fix compile warnings on 32-bit platforms,
> but actually broke the Windows build for 32-bit.

What compiler do you use? This problem doesn't happen for me with VCExpress, and the buildbot also seems happy.
Comment 5 Don Gibson 2006-11-06 10:16:19 PST
(In reply to comment #2)
> (From update of attachment 11369 [details] [edit])
> Looks good to me, but r- for style issues - pointer types should be written
> with no space between the type name and the *.

Sorry... was just trying to copy the existing file style.  Should I change this file to be the other way?

(In reply to comment #4)
> (In reply to comment #0)
> > The patch for bug 11504 purported to fix compile warnings on 32-bit platforms,
> > but actually broke the Windows build for 32-bit.
> 
> What compiler do you use? This problem doesn't happen for me with VCExpress,
> and the buildbot also seems happy.

Visual Studio 8 Pro.  Building Spinneret.sln broke on warnings-as-errors about the assigning of a 64-bit constant to an enum.
Comment 6 Alexey Proskuryakov 2006-11-06 12:41:21 PST
(In reply to comment #5)
> Sorry... was just trying to copy the existing file style.  Should I change this
> file to be the other way?

  I think I shouldn't have complained about that indeed - that's easier to fix globally when landing. But breaking Mac build with a Windows build fix is a major problem :)

> Visual Studio 8 Pro.  Building Spinneret.sln broke on warnings-as-errors about
> the assigning of a 64-bit constant to an enum.

  If possible, it would be nice to understand why BuildBot doesn't complain. Could it have a later version of VS 8?
Comment 7 Don Gibson 2006-11-08 16:10:34 PST
(In reply to comment #6)
> > Visual Studio 8 Pro.  Building Spinneret.sln broke on warnings-as-errors about
> > the assigning of a 64-bit constant to an enum.
> 
>   If possible, it would be nice to understand why BuildBot doesn't complain.
> Could it have a later version of VS 8?

Ah -- the reason Buildbot doesn't complain is because of revision 17587, which disabled warning 4341 for Windows builds.  I didn't have that when I filed this bug; that technically fixes the original bug here.  It would be nicer to not disable the warning, so I still think this patch is a good idea.  Resummarizing.

I don't have a solution to the gcc build issue, though; I don't have a Mac to try the build.  (Insert whine about gcc's crappy support for templates here.)  Can you come up with a fix for this?
Comment 8 Alexey Proskuryakov 2006-11-09 10:50:40 PST
Created attachment 11443 [details]
proposed patch

> (Insert whine about gcc's crappy support for templates here.)

I did a quick Google search, and it looks like gcc claims to follow the standard more closely here in fact.
Comment 9 Alexey Proskuryakov 2006-11-11 01:00:43 PST
Committed revision 17723.