Bug 11508 – Undisable some warnings for JSImmediate.h

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Don Gibson]

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.

[Don Gibson]

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.

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

Also, this breaks Mac build, because gcc doesn't like template specialization inside classes. I'll look into fixing this.

[Alexey Proskuryakov]

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

[Don Gibson]

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

[Alexey Proskuryakov]

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

[Don Gibson]

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Committed revision 17723.