1071100 - Move nsRefPtr out of nsAutoPtr into its own header file

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Right now, nsRefPtr is defined in nsAutoPtr.h, but it is quite unrelated to nsAutoPtr.  The first step would be to move nsRefPtr to a new nsRefPtr.h file in that same directory, and include nsRefPtr.h in nsAutoPtr.h.  I think that would be quite straight forward.

Then, a followup bug would involve fixing up the includes so we can remove the include of nsRefPtr.h from nsAutoPtr.h.  That could be quite awful, so I will file a separate bug for it.  Just having a separate nsRefPtr.h to include when you just want nsRefPtr would be an improvement already.

(This has annoyed me for a while, and Nathan mentioned it too, so I'll file a bug for this.)

Comment 1

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug1071100.diff

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8493894 [details] [diff] [review]
bug1071100.diff

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

::: xpcom/base/nsRefPtr.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef nsRefPtr_h___

nit: I know this is wrong in nsAutoPtr.h and you are just following that, but per the style guide ( https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices ) the include guard should be nsRefPtr_h, not nsRefPtr_h___.

Comment 3

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug1071100.diff

Fixed guard.

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

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8493915 [details] [diff] [review]
bug1071100.diff

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

Thanks for doing this!

::: xpcom/base/nsRefPtr.h
@@ +9,5 @@
> +
> +#include "nsCOMPtr.h"
> +
> +#include "nsCycleCollectionNoteChild.h"
> +#include "mozilla/MemoryReporting.h"

This #include is now unnecessary, since it's only there to support nsAutoArrayPtr.  Please remove it.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Andrew McCreight [:mccr8] from comment #0)
> Right now, nsRefPtr is defined in nsAutoPtr.h, but it is quite unrelated to
> nsAutoPtr.  The first step would be to move nsRefPtr to a new nsRefPtr.h
> file in that same directory, and include nsRefPtr.h in nsAutoPtr.h.  I think
> that would be quite straight forward.
> 
> Then, a followup bug would involve fixing up the includes so we can remove
> the include of nsRefPtr.h from nsAutoPtr.h.  That could be quite awful, so I
> will file a separate bug for it.

include-what-you-use (IWYU) could be an enormous help with that. https://blog.mozilla.org/nnethercote/2013/08/13/using-include-what-you-use/ has some details about how I used IWYU with Mozilla code, which may be useful.

Comment 6

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug1071100.diff r=froydnj

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

Comment 7

[Andrew McCreight [:mccr8]]

Anuj, I added back the includes of nsCOMPtr.h and nsCycleCollectionNoteChild.h, because froydnj was just talking about removing the memory reporter one, and those two are still needed for this header.

https://hg.mozilla.org/integration/mozilla-inbound/rev/39f6aa4ae636

Comment 8

[Andrew McCreight [:mccr8]]

I'll post about this on dev.platform when it merges, just so people know it is an option.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39f6aa4ae636

Comment 10

[Andrew McCreight [:mccr8]]

I just posted about it on dev-platform.