Last Comment Bug 781943 - [Azure] gfx::2d should be able to deal with nullptrs
: [Azure] gfx::2d should be able to deal with nullptrs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 15 Branch
: All All
: -- blocker (vote)
: mozilla17
Assigned To: Nick Cameron [:nrc]
:
:
Mentors:
: 782045 (view as bug list)
Depends on:
Blocks: 782416 782647
  Show dependency treegraph
 
Reported: 2012-08-10 14:45 PDT by Nick Cameron [:nrc]
Modified: 2012-08-14 07:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch: s/nullptr/NULL (793 bytes, patch)
2012-08-12 12:20 PDT, Nick Cameron [:nrc]
joe: review-
Details | Diff | Splinter Review
alternative patch: define nullptr macro (1.08 KB, patch)
2012-08-12 13:51 PDT, Nick Cameron [:nrc]
joe: review+
Details | Diff | Splinter Review

Description Nick Cameron [:nrc] 2012-08-10 14:45:12 PDT
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).
Comment 1 Philip Chee 2012-08-11 09:35:36 PDT
*** Bug 782045 has been marked as a duplicate of this bug. ***
Comment 2 Bill Gianopoulos [:WG9s] 2012-08-11 09:40:45 PDT
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.
Comment 3 Bill Gianopoulos [:WG9s] 2012-08-11 09:42:44 PDT
Not being able to build on a supported build environment is NOT a normal Importance bug.
Comment 4 Nick Cameron [:nrc] 2012-08-12 12:20:19 PDT
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).
Comment 5 Bill Gianopoulos [:WG9s] 2012-08-12 12:56:30 PDT
(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.
Comment 6 Nick Cameron [:nrc] 2012-08-12 13:03:47 PDT
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.
Comment 7 Nick Cameron [:nrc] 2012-08-12 13:51:43 PDT
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.
Comment 8 Nick Cameron [:nrc] 2012-08-12 13:52:25 PDT
Bas: just to clarify I only want you to r+ at most one of the patches, thanks :-)
Comment 9 Bill Gianopoulos [:WG9s] 2012-08-12 14:24:23 PDT
(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.
Comment 10 Bill Gianopoulos [:WG9s] 2012-08-12 14:25:39 PDT
(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.
Comment 11 Joe Drew (not getting mail) 2012-08-12 17:49:07 PDT
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.
Comment 12 Nick Cameron [:nrc] 2012-08-13 13:49:18 PDT
(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!
Comment 13 Bill Gianopoulos [:WG9s] 2012-08-13 13:58:09 PDT
(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.
Comment 14 Nick Cameron [:nrc] 2012-08-13 14:04:00 PDT
(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!
Comment 16 Ed Morley [:emorley] 2012-08-14 06:00:47 PDT
https://hg.mozilla.org/mozilla-central/rev/b441413e4c2d

Note You need to log in before you can comment on or make changes to this bug.