The default bug view has changed. See this FAQ.

[Azure] gfx::2d should be able to deal with nullptrs

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics
--
blocker
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

15 Branch
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

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

Updated

5 years ago
Duplicate of this bug: 782045
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.
Severity: normal → blocker
Not being able to build on a supported build environment is NOT a normal Importance bug.
(Assignee)

Comment 4

5 years ago
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).
Assignee: nobody → ncameron
Attachment #651217 - Flags: review?(bas.schouten)
(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.
(Assignee)

Comment 6

5 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.
(Assignee)

Comment 7

5 years ago
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.
Attachment #651226 - Flags: review?(bas.schouten)
(Assignee)

Comment 8

5 years ago
Bas: just to clarify I only want you to r+ at most one of the patches, thanks :-)
(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.
(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 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.
Attachment #651226 - Flags: review?(bas.schouten) → review+
Attachment #651217 - Flags: review?(bas.schouten) → review-
(Assignee)

Comment 12

5 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!
(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.
(Assignee)

Comment 14

5 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!
(Assignee)

Comment 15

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b441413e4c2d
(Assignee)

Updated

5 years ago
Blocks: 782416
https://hg.mozilla.org/mozilla-central/rev/b441413e4c2d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Blocks: 782647
You need to log in before you can comment on or make changes to this bug.