Nick Cameron [:nrc] Assignee
	Description • 12 years ago

The Azure files do not include nscore.h so do not have a defined nullptr macros, so compiling in a compiler which does not support nullptr gives an error if nullptr is used in gfx::2d (which it is, e.g., http://mxr.mozilla.org/mozilla-central/source/gfx/2d/ScaledFontDWrite.h#36).

	Bill Gianopoulos [:WG9s]
	Comment 2 • 12 years ago

k since my blocker bug was duped to this because it prevents me from working on issues in windows build I am updating this to blocker as well.

	Bill Gianopoulos [:WG9s]
	Comment 3 • 12 years ago

Not being able to build on a supported build environment is NOT a normal Importance bug.

	Nick Cameron [:nrc] Assignee
	Comment 4 • 12 years ago

If we don't want to use nullptr in gfx::2d, then this patch fixes the compile bug. Also a temporary fix for those with older compilers who can't build. If we do want to fix this, we don't want this patch. (Note, there is still nullptr use in QuartzSupport.mm but this is Objective C, so presumably doesn't cause a problem).

	Bill Gianopoulos [:WG9s]
	Comment 5 • 12 years ago

(In reply to Nick Cameron [:nrc] from comment #4)
> Created attachment 651217 [details] [diff] [review]
> patch: s/nullptr/NULL
> 
> If we don't want to use nullptr in gfx::2d, then this patch fixes the
> compile bug. Also a temporary fix for those with older compilers who can't
> build. If we do want to fix this, we don't want this patch. (Note, there is
> still nullptr use in QuartzSupport.mm but this is Objective C, so presumably
> doesn't cause a problem).

This is essentially the same patch I have used in my personal builds for the last 2 days, yet I don;t think it is the right thing to do either.

My main point here was that we have a problem because the Windows build prerequisites say that you can use VC8, VC9 or VC10, yet with this issue, only VC10 can actually build successfully.

This will be an issue for many of the contributors who reliquary work on the codebase and submit patches for the Windows platform.

	Nick Cameron [:nrc] Assignee
	Comment 6 • 12 years ago

Hi Bill, I'm working on a proper fix right now, the first patch is just a stopgap measure.

However, we may decide to not support nullptr in gfx::2d in which case we need that patch. That is because gfx::2d is designed to build standalone, so has a set of issues not found with the rest of the codebase. Waiting for a call from Bas as to what we end up doing.

One way or another we will make sure we can build using all supported flavours of VC.

	Nick Cameron [:nrc] Assignee
	Comment 7 • 12 years ago

If we want to use nullptr in gfx::2d, I guess we need something like this. It's a bit of a copy-and-paste hack job, but without including Mozilla libs, I'm not sure how to do better (I guess we share from MFBT or one of the libs we are using). I'm guessing when building standalone, we only target C++11, so we don't want to define nullptr, which is why I've stuck the MOZ_GFX test there, assuming that won't be defined when we build standalone, but I suspect it might be, in which case we (probably) need another define.

If we do this, we should probably change all the other NULLs in gfx::2d to nullptr.

I've tested this as best I could, but don't have an older version of VC to test with. If someone with VC9 or below could try to build with just the second patch and let me know if it works, I would be very grateful.

	Nick Cameron [:nrc] Assignee
	Comment 8 • 12 years ago

Bas: just to clarify I only want you to r+ at most one of the patches, thanks :-)

	Bill Gianopoulos [:WG9s]
	Comment 9 • 12 years ago

(In reply to Nick Cameron [:nrc] from comment #7)
> Created attachment 651226 [details] [diff] [review]
> alternative patch: define nullptr macro
> 
> If we want to use nullptr in gfx::2d, I guess we need something like this.
> It's a bit of a copy-and-paste hack job, but without including Mozilla libs,
> I'm not sure how to do better (I guess we share from MFBT or one of the libs
> we are using). I'm guessing when building standalone, we only target C++11,
> so we don't want to define nullptr, which is why I've stuck the MOZ_GFX test
> there, assuming that won't be defined when we build standalone, but I
> suspect it might be, in which case we (probably) need another define.
> 
> If we do this, we should probably change all the other NULLs in gfx::2d to
> nullptr.
> 
> I've tested this as best I could, but don't have an older version of VC to
> test with. If someone with VC9 or below could try to build with just the
> second patch and let me know if it works, I would be very grateful.

My opinion, for whatever that is worth, is that this patch fixes the issue as described in this bug description.  The other patch only fixes the issue I reported in bug 782045.

	Bill Gianopoulos [:WG9s]
	Comment 10 • 12 years ago

(In reply to Bill Gianopoulos [:WG9s] from comment #9)
> (In reply to Nick Cameron [:nrc] from comment #7)
> > Created attachment 651226 [details] [diff] [review]
> > alternative patch: define nullptr macro
> > If we do this, we should probably change all the other NULLs in gfx::2d to
> > nullptr.

But that could be done in a followup patch.

	Joe Drew (not getting mail)
	Comment 11 • 12 years ago

Comment on attachment 651226 [details] [diff] [review]
alternative patch: define nullptr macro

As long as this doesn't interfere with the definition elsewhere, let's do this. NULL is old and busted.

	Nick Cameron [:nrc] Assignee
	Comment 12 • 12 years ago

(In reply to Bill Gianopoulos [:WG9s] from comment #9)
> (In reply to Nick Cameron [:nrc] from comment #7)
> > Created attachment 651226 [details] [diff] [review]
> > alternative patch: define nullptr macro
> > 
> > If we want to use nullptr in gfx::2d, I guess we need something like this.
> > It's a bit of a copy-and-paste hack job, but without including Mozilla libs,
> > I'm not sure how to do better (I guess we share from MFBT or one of the libs
> > we are using). I'm guessing when building standalone, we only target C++11,
> > so we don't want to define nullptr, which is why I've stuck the MOZ_GFX test
> > there, assuming that won't be defined when we build standalone, but I
> > suspect it might be, in which case we (probably) need another define.
> > 
> > If we do this, we should probably change all the other NULLs in gfx::2d to
> > nullptr.
> > 
> > I've tested this as best I could, but don't have an older version of VC to
> > test with. If someone with VC9 or below could try to build with just the
> > second patch and let me know if it works, I would be very grateful.
> 
> My opinion, for whatever that is worth, is that this patch fixes the issue
> as described in this bug description.  The other patch only fixes the issue
> I reported in bug 782045.

Bill: would you mind testing this (patch 2) and seeing if it cures your build error? I don't really want to land the patch without knowing it works. Thanks!

	Bill Gianopoulos [:WG9s]
	Comment 13 • 12 years ago

(In reply to Nick Cameron [:nrc] from comment #12)
> Bill: would you mind testing this (patch 2) and seeing if it cures your
> build error? I don't really want to land the patch without knowing it works.
> Thanks!

Already done!  The builds at http://www.wg9s.com/mozilla/firefox/ were all built including this patch.

They work fine for me.

	Nick Cameron [:nrc] Assignee
	Comment 14 • 12 years ago

(In reply to Bill Gianopoulos [:WG9s] from comment #13)
> (In reply to Nick Cameron [:nrc] from comment #12)
> > Bill: would you mind testing this (patch 2) and seeing if it cures your
> > build error? I don't really want to land the patch without knowing it works.
> > Thanks!
> 
> Already done!  The builds at http://www.wg9s.com/mozilla/firefox/ were all
> built including this patch.
> 
> They work fine for me.

Oh great, thanks!

	Nick Cameron [:nrc] Assignee
	Comment 15 • 12 years ago

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b441413e4c2d

	Ed Morley [:emorley]
	Comment 16 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/b441413e4c2d