626472 - Switch from nsnull to nullptr

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

This could probably be done wholesale.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://stackoverflow.com/questions/2419800/can-nullptr-be-emulated-in-gcc

We should replace usage of both NULL and nsnull with nullptr. This would be great!

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm all for it.  Should be a pretty straightforward rewrite except for NULL with varargs, like printf("%p", NULL), but those should be taken out back and shot anyway.

Comment 3

[(no longer active)]

So, is there a way to detect if a compiler has built-in support for nullptr or not, to know when to use the workaround in comment 1?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You can use a configure check.

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I have a patch to do this, which I've pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=3ebce4c97955

The template workaround isn't great, because while it allows implicit conversion to raw pointers, it doesn't allow implicit conversion to smart pointers like already_AddRefed.  Rather than add more implicit conversions to the workaround, which would break whenever someone added new smart pointer classes but only on old compilers, I opted to leave all old compilers with the old 0L/0LL definitions (along with C code).

Some of the errors I had to fix argue fairly strongly in favor of this change, to say the least.  Like, there were a couple of places where nsnull was being used to mean NS_OK.  Overall, I had to change nsnull in various places to the following:

* 0
* '\0'
* NPERR_NO_ERROR (confusing!)
* NS_OK (confusing!)
* kNameSpaceID_None (arguably more cute than confusing)

I just picked whatever seemed most appropriate.  There's no actual difference between them, after all.

The patch makes "nullptr" and "nsnull" mean the same thing.  A later patch could change all nsnull to nullptr, but that would be a pretty big patch!

We should also change all callers of NULL to use nullptr.  Ideally we should undefine NULL, or define it as use_nullptr_instead_of_NULL so that the compilation error is more informative, but maybe that would break third-party code.

I'll ask for review once the patch passes try, since I'm not confident it will compile on all platforms we test.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to :Aryeh Gregor from comment #5)
> The patch makes "nullptr" and "nsnull" mean the same thing.  A later patch
> could change all nsnull to nullptr, but that would be a pretty big patch!

That would be a great patch :-).

Thanks a ton!

Comment 7

[(no longer active)]

This is great!  :-)

Make sure to test Mac builds using both clang and gcc 4.2 to ensure that your patch will stick if it lands during the 17 cycle.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> (In reply to :Aryeh Gregor from comment #5)
> > The patch makes "nullptr" and "nsnull" mean the same thing.  A later patch
> > could change all nsnull to nullptr, but that would be a pretty big patch!
> 
> That would be a great patch :-).
> 
> Thanks a ton!

Would this be good enough, assuming it compiles?

  for FILE in `find * -name '*.cpp' -o -name '*.h' -o -name '*.cc' -o -name '*.py' -o -name '*.idl' -o -name '*.mm'`; do sed -i 's/\bnsnull\b/nullptr/g' $FILE; done

(I tried using find -exec, but it missed some files.)  This might conceivably change things we don't want to change, but it's probably not a problem if it compiles.  A trial run indicates on the order of 10,000 lines changed, so I don't think it's actually reviewable.  On the other hand, there's no real reason anything should use the string "nsnull" after this patch lands.

How would I go about landing this?  Should the tree be closed briefly, as was apparently the case with the PRBool change, so that people don't accidentally land things that break?

(And no, the initial patch didn't compile on Mac.  So I have to repeatedly play whack-a-mole using the try server to compile.  Oh well.)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds reasonable. We probably don't need to close the tree, during a quiet time you could pull from inbound, run the script and push without losing a race. Obviously you'd land your substantive patch first and make sure it sticks before doing the mass change.

Comment 10

[Joshua Cranmer [:jcranmer]]

(In reply to :Aryeh Gregor from comment #8)
> (And no, the initial patch didn't compile on Mac.  So I have to repeatedly
> play whack-a-mole using the try server to compile.  Oh well.)

You'd probably also want to test linux builds with clang or gcc-4.6, as well as MSVC 10 on Windows too to see who breaks there as well.

Comment 11

[(no longer active)]

(In reply to comment #8)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> > (In reply to :Aryeh Gregor from comment #5)
> > > The patch makes "nullptr" and "nsnull" mean the same thing.  A later patch
> > > could change all nsnull to nullptr, but that would be a pretty big patch!
> > 
> > That would be a great patch :-).
> > 
> > Thanks a ton!
> 
> Would this be good enough, assuming it compiles?
> 
>   for FILE in `find * -name '*.cpp' -o -name '*.h' -o -name '*.cc' -o -name
> '*.py' -o -name '*.idl' -o -name '*.mm'`; do sed -i 's/\bnsnull\b/nullptr/g'
> $FILE; done

Hmm, see my sed script in bug 690892, for example.  I suggest you attach a bz2 patch and your sed script, and request dbaron for review (he actually spotted a problem with my patch there)!

Also if you keep nsnull working, there should not be a need to close the tree, although providing scripts for people to bitrot their patches and a post to dev-platform before doing it is probably a good idea.

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1 -- Define nsnull as nullptr where available

Green try run (compile only): https://tbpl.mozilla.org/?tree=Try&rev=0e3a4d40da3b

Older try run that's busted on Mac and was aborted partway through, but seems greenish on Linux for all tests: https://tbpl.mozilla.org/?tree=Try&rev=3ebce4c97955

So almost all of this patch is just changing incorrect nsnull uses to other things, as documented in comment 5.  The interesting bits are configure.in and xpcom/base/nscore.h.

storage/test/test_AsXXX_helpers.cpp is worthy of comment: I changed nsnull to NULL.  The reason is explained here: <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54043>.  There's no specific overload for ostream<<(nullptr_t), so you can't send a nullptr to an ostream with << -- it complains that the overloads are ambiguous.  This is probably a bug in the C++ spec, which I'll report.  For now, since this file uses a macro that sends its arguments to an ostream, it can't use nsnull, but NULL still works.


How do you want me to handle the next patch in the series, namely s/nsnull/nullptr/?  Is it okay to just do that with sed, make sure it compiles on m-c (including try), redo it on m-i, push with r=you, and see what happens?

Comment 13

[(no longer active)]

Comment on attachment 644300 [details] [diff] [review]
Patch part 1 -- Define nsnull as nullptr where available

Review of attachment 644300 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +481,5 @@
>  NS_IMETHODIMP
>  HyperTextAccessible::GetCharacterAtOffset(PRInt32 aOffset, PRUnichar* aCharacter)
>  {
>    NS_ENSURE_ARG_POINTER(aCharacter);
> +  *aCharacter = '\0';

Nit: L'\0' for consistency with the type.

Comment 14

[(no longer active)]

(In reply to :Aryeh Gregor from comment #12)
> How do you want me to handle the next patch in the series, namely
> s/nsnull/nullptr/?  Is it okay to just do that with sed, make sure it
> compiles on m-c (including try), redo it on m-i, push with r=you, and see
> what happens?

Please ask someone to review your sed script, and once you do that, then this seems fine!

Comment 15

[Masatoshi Kimura [:emk]]

(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> Nit: L'\0' for consistency with the type.
I think it should be PRUnichar('\0') unless it is inside Windows-specific code.

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2 -- Change all nsnull to nullptr

This was created via:

  for FILE in `hg stat -mardc | sed 's/^. //'`; do \
    sed -i 's/\bnsnull\b/nullptr/g' $FILE; \
  done; \
  hg revert -C xpcom/base/nscore.h

The hg stat command lists the status of all tracked files.  sed then strips the leading "M ", "A ", "R ", "D ", or "C ", so you have just the filenames.  Then the sed -i invocation changes every whole word "nsnull" with "nullptr".  Finally, it reverts nscore.h because the only occurrence of nsnull there is "#define nsnull nullptr", which is silly to convert.  The file is gzipped because it's 11M raw, which is over the size limit.

This compiles successfully on localhost, and I'm pushing it to try to see how it goes.  Obviously, r+ here doesn't mean trying to apply this exact patch, since practically any change will bitrot it instantly.  I would instead run the same command on m-i and push the resulting patch there, after coordinating with whoever was planning on doing the next merge from m-i to m-c, because when merging the same sed commands will have to be run.


Issues with the general idea here:

* This will seriously clutter hg blames forever.  It's over 20,000 lines changed, with no real difference to the meaning of the changed lines.  This means who knows how many people will have to manually redo the blame to get to the real changed lines.
* Likewise, this will immediately bitrot most out-of-tree patches.  This is one-time and easily fixed by running sed on the patches, though.

Should this be discussed anywhere, or are you okay with me pushing this as-is?  By comparison, the PRBool switchover in September was over 30,000 lines changed, so if people were okay with that, this should be fine too.  I just want to make extra sure that if people yell at anyone, it's you and not me.  ;)

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3 -- The final demise of nsnull

Once we've removed all users, no need to keep it defined.  Try: https://tbpl.mozilla.org/?tree=Try&rev=859ce123e7c2

Comment 18

[Joshua Cranmer [:jcranmer]]

(In reply to :Aryeh Gregor from comment #17)
> Created attachment 644932 [details] [diff] [review]
> Patch part 3 -- The final demise of nsnull
> 
> Once we've removed all users, no need to keep it defined.  Try:
> https://tbpl.mozilla.org/?tree=Try&rev=859ce123e7c2

I presume you're also planning on switching comm-central, then?

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The script from comment 16 (minus the hg revert at the end) should be just as easy to apply to comm-central as mozilla-central.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to :Aryeh Gregor from comment #16)
> patch, since practically any change will bitrot it instantly.  I would
> instead run the same command on m-i and push the resulting patch there,
> after coordinating with whoever was planning on doing the next merge from
> m-i to m-c, because when merging the same sed commands will have to be run.


Maybe I'm missing something, but why would you have to re-run the sed commands? Here's what I'd propose, tell me why it wouldn't work.

At a time when the tree isn't busy (weekends FTW), close both m-c and inbound. Get both trees in sync (easy enough to do, we do it every day). Land on m-c. Merge it over to inbound. As long as both trees are closed so nothing's landing in the mean time, I don't see why you'd have you to do any sed gymnastics. And FWIW, IIRC, that's how the PRBool change was landed.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

(And re-open the trees after both go green, of course :P)

Comment 22

[(no longer active)]

Comment on attachment 644931 [details]
Patch part 2 -- Change all nsnull to nullptr

You have a change to ipc/chromium which we don't own.  (The script which I pointed to in bug 690892 protects against this, I'm not sure why you did not use it.)

Comment 23

[(no longer active)]

(In reply to :Aryeh Gregor from comment #16)
> This compiles successfully on localhost, and I'm pushing it to try to see
> how it goes.  Obviously, r+ here doesn't mean trying to apply this exact
> patch, since practically any change will bitrot it instantly.  I would
> instead run the same command on m-i and push the resulting patch there,
> after coordinating with whoever was planning on doing the next merge from
> m-i to m-c, because when merging the same sed commands will have to be run.

As far as the landing plan goes, I suggest that you should land first on mozilla-central and then immediately merge central into inbound.  If you have tested your patch on try, the biggest pain when landing it will be the merge itself, so if you merge to inbound immediately, it should be smooth.  The reason that I think you should land on central directly and then merge to inbound is that merging from inbound to central is a lot harder (you need to wait for green PGO builds, etc.)

> * This will seriously clutter hg blames forever.  It's over 20,000 lines
> changed, with no real difference to the meaning of the changed lines.  This
> means who knows how many people will have to manually redo the blame to get
> to the real changed lines.

That's fine, you almost have to do multiple blame passes anyways.

> * Likewise, this will immediately bitrot most out-of-tree patches.  This is
> one-time and easily fixed by running sed on the patches, though.

You should attach a patch to this bug which people can use to unbitrot their patch queue (as I mentioned in comment 11)

> Should this be discussed anywhere, or are you okay with me pushing this
> as-is?  By comparison, the PRBool switchover in September was over 30,000
> lines changed, so if people were okay with that, this should be fine too.  I
> just want to make extra sure that if people yell at anyone, it's you and not
> me.  ;)

You should definitely post to dev.platform a few days before landing this, and you should point them to the script which I mentioned above.

Comment 24

[(no longer active)]

Comment on attachment 644932 [details] [diff] [review]
Patch part 3 -- The final demise of nsnull

r=me if this only lands after c-c has been converted as well.

Comment 25

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Ehsan Akhgari [:ehsan] from comment #22)
> You have a change to ipc/chromium which we don't own.  (The script which I
> pointed to in bug 690892 protects against this, I'm not sure why you did not
> use it.)

I'm confused -- if we don't own it, why does it use nsnull to start with?  The nsnull was added by bug 723846.  If that can add it, why can't we take it out?

(In reply to Ehsan Akhgari [:ehsan] from comment #23)
> As far as the landing plan goes, I suggest that you should land first on
> mozilla-central and then immediately merge central into inbound.  If you
> have tested your patch on try, the biggest pain when landing it will be the
> merge itself, so if you merge to inbound immediately, it should be smooth. 
> The reason that I think you should land on central directly and then merge
> to inbound is that merging from inbound to central is a lot harder (you need
> to wait for green PGO builds, etc.)

Would it make sense to wait until m-c is merged to m-i, then push the patch to m-c and then merge it immediately to m-i as well?  That way there's no actual merging.  Perhaps a closed tree would be simplest, as Ryan suggests.

> You should attach a patch to this bug which people can use to unbitrot their
> patch queue (as I mentioned in comment 11)

Okay.

> You should definitely post to dev.platform a few days before landing this,
> and you should point them to the script which I mentioned above.

Okay, I'll make a post tomorrowish.

(In reply to Ehsan Akhgari [:ehsan] from comment #24)
> r=me if this only lands after c-c has been converted as well.

Okay -- although really, if we can't break c-c, remind me why it's in a different tree again . . .

Comment 26

[Joshua Cranmer [:jcranmer]]

(In reply to Ehsan Akhgari [:ehsan] from comment #24)
> Comment on attachment 644932 [details] [diff] [review]
> Patch part 3 -- The final demise of nsnull
> 
> r=me if this only lands after c-c has been converted as well.

The optimum time to convert c-c is when the tree is closed for the m-c conversion. So close Mozilla-*, Thunderbird-Trunk and SeaMonkey, land on m-c/m-i, then land on c-c (which would pick up m-c's changeset as well), wait for all to go green, reopen.

Comment 27

[(no longer active)]

(In reply to :Aryeh Gregor from comment #25)
> (In reply to Ehsan Akhgari [:ehsan] from comment #22)
> > You have a change to ipc/chromium which we don't own.  (The script which I
> > pointed to in bug 690892 protects against this, I'm not sure why you did not
> > use it.)
> 
> I'm confused -- if we don't own it, why does it use nsnull to start with? 
> The nsnull was added by bug 723846.  If that can add it, why can't we take
> it out?

Actually glandium told me on IRC that I'm wrong, but I remember somebody else telling this to me a while ago (don't remmeber who.)  cjones: can we take patches to ipc/chromium?

> (In reply to Ehsan Akhgari [:ehsan] from comment #23)
> > As far as the landing plan goes, I suggest that you should land first on
> > mozilla-central and then immediately merge central into inbound.  If you
> > have tested your patch on try, the biggest pain when landing it will be the
> > merge itself, so if you merge to inbound immediately, it should be smooth. 
> > The reason that I think you should land on central directly and then merge
> > to inbound is that merging from inbound to central is a lot harder (you need
> > to wait for green PGO builds, etc.)
> 
> Would it make sense to wait until m-c is merged to m-i, then push the patch
> to m-c and then merge it immediately to m-i as well?  That way there's no
> actual merging.  Perhaps a closed tree would be simplest, as Ryan suggests.

That could still prove the next merge difficult.  The point here is that you should be the person doing the merge, not edmorley, etc.  I think what I suggested above should work without a problem.  (I'm not entirely opposed to close the tree for a few hours, but I don't think it's necessary if we land part 3 a few days after the first two parts land.)

> (In reply to Ehsan Akhgari [:ehsan] from comment #24)
> > r=me if this only lands after c-c has been converted as well.
> 
> Okay -- although really, if we can't break c-c, remind me why it's in a
> different tree again . . .

Long story, you don't wanna go there...  ;-)

(Hint: I really don't know -- people tell me it's easier for c-c folks, I think they're wrong!)

Comment 28

[(no longer active)]

CCing a bunch of c-c folks...

Comment 29

[Ryan VanderMeulen [:RyanVM]]

1.) I'm willing to help shepherd this in on a Saturday morning when the tree isn't busy and nobody's going to mind the tree being closed for a few hours. That is conditional on Aryeh being around to fix any bustage that crops up as needed.

2.) I would prefer we land on inbound first to keep m-c in a known good state at all times. That way any follow-up bustage fixes can also land before we merge it over and re-open the trees. Waiting for PGO builds isn't the end of the world.

3.) As jcranmer said, we should land a c-c patch at the same time as we do the inbound-->m-c merge. I would also recommend we clobber said trees before landing on them.

4.) It would also be good for the owners of fx-team and services (any other project branches that merge regularly to m-c that I'm forgetting?) to sync with m-c ahead of time as they may want to avoid landing on their trees as well to avoid later merge pain.

Would that work for everyone?

Comment 30

[(no longer active)]

(In reply to comment #29)
> 1.) I'm willing to help shepherd this in on a Saturday morning when the tree
> isn't busy and nobody's going to mind the tree being closed for a few hours.
> That is conditional on Aryeh being around to fix any bustage that crops up as
> needed.

That's fine by me, but I don't know if Aryeh will be available.

> 2.) I would prefer we land on inbound first to keep m-c in a known good state
> at all times. That way any follow-up bustage fixes can also land before we
> merge it over and re-open the trees. Waiting for PGO builds isn't the end of
> the world.

OK but that requires keeping inbound closed for what perhaps 8 hours?  (The time to go Windows PGO builds and run tests on them).

> 3.) As jcranmer said, we should land a c-c patch at the same time as we do the
> inbound-->m-c merge. I would also recommend we clobber said trees before
> landing on them.

Only if want to land part 3 as well, which we don't have to.

> 4.) It would also be good for the owners of fx-team and services (any other
> project branches that merge regularly to m-c that I'm forgetting?) to sync with
> m-c ahead of time as they may want to avoid landing on their trees as well to
> avoid later merge pain.

The dev.platform post should take care of that.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ehsan Akhgari [:ehsan] from comment #30)
> (In reply to comment #29)
> > 2.) I would prefer we land on inbound first to keep m-c in a known good state
> > at all times. That way any follow-up bustage fixes can also land before we
> > merge it over and re-open the trees. Waiting for PGO builds isn't the end of
> > the world.
> 
> OK but that requires keeping inbound closed for what perhaps 8 hours?  (The
> time to go Windows PGO builds and run tests on them).
> 

~4 hours if the tree isn't busy.

> > 3.) As jcranmer said, we should land a c-c patch at the same time as we do the
> > inbound-->m-c merge. I would also recommend we clobber said trees before
> > landing on them.
> 
> Only if want to land part 3 as well, which we don't have to.
> 

No problem with me either way.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 644300 [details] [diff] [review]
Patch part 1 -- Define nsnull as nullptr where available

Looks like Aryeh landed this earlier today and didn't update the bug?

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5b58d92843

Comment 33

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Ryan VanderMeulen from comment #29)
> 1.) I'm willing to help shepherd this in on a Saturday morning when the tree
> isn't busy and nobody's going to mind the tree being closed for a few hours.
> That is conditional on Aryeh being around to fix any bustage that crops up
> as needed.

I can only be around Saturday nights my time (UTC+0200 right now due to DST), not Saturday mornings.  Also, this Saturday night doesn't work for me, although next week would.

> 4.) It would also be good for the owners of fx-team and services (any other
> project branches that merge regularly to m-c that I'm forgetting?) to sync
> with m-c ahead of time as they may want to avoid landing on their trees as
> well to avoid later merge pain.

Running the provided sed script on the tree should avoid merge pain, right?  Or will merging still be a pain if two commits to the different trees make the exact same changes?  (I pretty much never merge anything, just rebase, so I'm honestly not so familiar with the procedure.)

(In reply to Ehsan Akhgari [:ehsan] from comment #30)
> Only if want to land part 3 as well, which we don't have to.

Well, we'd hit the same problem when landing part 3 later, assuming we do.

(In reply to Ryan VanderMeulen from comment #32)
> Comment on attachment 644300 [details] [diff] [review]
> Patch part 1 -- Define nsnull as nullptr where available
> 
> Looks like Aryeh landed this earlier today and didn't update the bug?
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5b58d92843

Oops, sorry about that!

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Ehsan Akhgari [:ehsan] from comment #27)
> (In reply to :Aryeh Gregor from comment #25)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #22)
> > > You have a change to ipc/chromium which we don't own.  (The script which I
> > > pointed to in bug 690892 protects against this, I'm not sure why you did not
> > > use it.)
> > 
> > I'm confused -- if we don't own it, why does it use nsnull to start with? 
> > The nsnull was added by bug 723846.  If that can add it, why can't we take
> > it out?
> 
> Actually glandium told me on IRC that I'm wrong, but I remember somebody
> else telling this to me a while ago (don't remmeber who.)  cjones: can we
> take patches to ipc/chromium?

We can; we've forked ipc/chromium long ago.

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0a5b58d92843

Comment 36

[Ryan VanderMeulen [:RyanVM]]

(In reply to :Aryeh Gregor from comment #33)
> I can only be around Saturday nights my time (UTC+0200 right now due to
> DST), not Saturday mornings.  Also, this Saturday night doesn't work for me,
> although next week would.
> 

I'm UTC-0400, so my morning is your afternoon. So if we were to shoot for evening your time, we'd be looking at closing the tree in the late morning/early afternoon in the US. Based on my experience with weekend sheriffing, I really don't see that as a big deal given the relatively low volume of commits, but I'll leave it to the powers that be to officially say OK to it.

> Running the provided sed script on the tree should avoid merge pain, right? 
> Or will merging still be a pain if two commits to the different trees make
> the exact same changes?  (I pretty much never merge anything, just rebase,
> so I'm honestly not so familiar with the procedure.)
> 

I'm thinking it would be preferable to avoid, but that's really a call for the maintainers of those various trees. I know that if I were maintaining a tree that tracked m-c, I'd probably just sync with m-c prior to the landing and avoid pushing anything new to my tree until after m-c was reopened. Seems like less hassle all around.

> (In reply to Ehsan Akhgari [:ehsan] from comment #30)
> > Only if want to land part 3 as well, which we don't have to.
> 
> Well, we'd hit the same problem when landing part 3 later, assuming we do.
> 

Yeah, I'd rather land 2 and 3 at the same time so we don't get people landing more nsnull on the tree in the mean time.

Comment 37

[(not currently active) Ted Mielczarek]

I think you guys should find whatever time is most convenient for you and make it happen.

Comment 38

[(no longer active)]

I think we're making this a lot more complicated than it needs to be.  This really doesn't require tree closure.  Here's a suggestion: just ping me some time in your afternoon which will be my morning, and push to m-c.  I'll merge it back to inbound and I'll watch both trees and spot fix any build failures.  Of course we need to do this once you've posted to dev.platform about it (and it would be nice to give people a few days notice in advance.)

(Also note that I'll be offline next week Wednesday and Friday, and the Monday after that is a holiday here, so I suggest you try to shoot for the next Monday at the latest if you require my assistance.)

Comment 39

[(no longer active)]

(In reply to Ryan VanderMeulen from comment #36)
> > Running the provided sed script on the tree should avoid merge pain, right? 
> > Or will merging still be a pain if two commits to the different trees make
> > the exact same changes?  (I pretty much never merge anything, just rebase,
> > so I'm honestly not so familiar with the procedure.)
> > 
> 
> I'm thinking it would be preferable to avoid, but that's really a call for
> the maintainers of those various trees. I know that if I were maintaining a
> tree that tracked m-c, I'd probably just sync with m-c prior to the landing
> and avoid pushing anything new to my tree until after m-c was reopened.
> Seems like less hassle all around.

The merge should only be a problem if they have changed code around a place where nsnull is used on their branches, and fixing the conflict would be easy, and it is the responsibility of the maintainer of those respective trees.  Really I think we just need to post about this to dev.platform.  Everything else is the responsibility of those maintainers, and I would be happy to help them (if they don't ask me to clone their repos ;-)

> > (In reply to Ehsan Akhgari [:ehsan] from comment #30)
> > > Only if want to land part 3 as well, which we don't have to.
> > 
> > Well, we'd hit the same problem when landing part 3 later, assuming we do.
> > 
> 
> Yeah, I'd rather land 2 and 3 at the same time so we don't get people
> landing more nsnull on the tree in the mean time.

That wouldn't be very nice.  People do have nsnulls in their patches and they will forget to take them out.  What we should do is to give them a couple of weeks after landing part 2, and then run the sed script again and land that together with part 3 and close this bug.  Removing nsnull immediately will only result in a constantly burning tree which doesn't help anyone.  We need to give people time to get into the habit of using nullptr.  :-)

Comment 40

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Script to unbitrot patch dir (Linux/OS X)

Stolen from bug 690892.  Windows users can use their favorite regex program instead.

Comment 41

[(no longer active)]

Aryeh landed part 2 on mozilla-central: https://hg.mozilla.org/mozilla-central/rev/b5c4b792f3f2

Comment 42

[Michael Wu [:mwu]]

(In reply to :Aryeh Gregor from comment #40)
> Created attachment 647139 [details]
> Script to unbitrot patch dir (Linux/OS X)
> 
> Stolen from bug 690892.  Windows users can use their favorite regex program
> instead.

mozbuild provides an old gnu sed. The script from bug 690892 works on all three platforms, so your script should too.

Comment 43

[(no longer active)]

Aryeh, when you have some time, I think we can remove any occurrences of nsnull that have crept in and then remove it from mozilla-central.

Comment 44

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3, v2 -- The final demise of nsnull

Fixes generated with

  for file in `hg stat -mardcn`; do sed -i 's/\bnsnull\b/nullptr/g' $file; done

I'll rerun it on inbound just before pushing to make sure nothing was missed.

Comment 45

[(no longer active)]

Comment on attachment 650512 [details] [diff] [review]
Patch part 3, v2 -- The final demise of nsnull

Review of attachment 650512 [details] [diff] [review]:
-----------------------------------------------------------------

Please post to dev.platform once this lands.  Also, again, I would recommend landing on central and merging central into inbound to reduce the merge pain.

Comment 46

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Posting to dev.platform now.

Comment 47

[(no longer active)]

I removed one last nsnull that had crept into mozilla-inbound before merging:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7211dc2af811

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7211dc2af811

Comment 49

[(no longer active)]

I filed bug 784739 to switch NULL to nullptr as well.

Comment 50

[Hans-Peter Budek]

Attachment: replace nullptr with '0' or '\0' where necessary

Comment 51

[Hans-Peter Budek]

The switch to 'nullptr' has erroneous changed '0' or '\0' to
nullptr in some places. This prevents the compilation of
seamonkey-2.14.1 on my 64-bit system.
The attached patch corrects this.

Comment 52

[Mark Banner (:standard8)]

(In reply to Hans-Peter Budek from comment #51)
> The switch to 'nullptr' has erroneous changed '0' or '\0' to
> nullptr in some places. This prevents the compilation of
> seamonkey-2.14.1 on my 64-bit system.
> The attached patch corrects this.

Hi Hans-Peter, I think you'll find these have already been fixed in bug 789827 for the next version. In general it is best to check the latest trunk (i.e. comm-central to see if issues are still there or not), and also please file new bugs if necessary - comments on closed bugs can frequently get lost due to the closed state.