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.
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 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).
Also, this breaks Mac build, because gcc doesn't like template specialization inside classes. I'll look into fixing this.
(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.
(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.
(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?
(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?
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.
Committed revision 17723.