Closed Bug 1071100 Opened 6 years ago Closed 6 years ago

Move nsRefPtr out of nsAutoPtr into its own header file

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: mccr8, Assigned: anujagarwal464, Mentored)

References

Details

Attachments

(1 file, 2 obsolete files)

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.)
Blocks: 1071102
Assignee: nobody → anujagarwal464
Attached patch bug1071100.diff (obsolete) — Splinter Review
Attachment #8493894 - Flags: feedback?(continuation)
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___.
Attachment #8493894 - Flags: feedback?(continuation) → feedback+
Attached patch bug1071100.diff (obsolete) — Splinter Review
Fixed guard.

Try : https://tbpl.mozilla.org/?tree=Try&rev=3a3d372ff6a5
Attachment #8493894 - Attachment is obsolete: true
Attachment #8493915 - Flags: review?(continuation)
Attachment #8493915 - Flags: review?(continuation) → review?(nfroyd)
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.
Attachment #8493915 - Flags: review?(nfroyd) → review+
(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.
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
I'll post about this on dev.platform when it merges, just so people know it is an option.
Flags: needinfo?(continuation)
https://hg.mozilla.org/mozilla-central/rev/39f6aa4ae636
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
I just posted about it on dev-platform.
Flags: needinfo?(continuation)
You need to log in before you can comment on or make changes to this bug.