Closed
Bug 1071100
Opened 10 years ago
Closed 10 years ago
Move nsRefPtr out of nsAutoPtr into its own header file
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: mccr8, Assigned: anujagarwal464, Mentored)
References
Details
Attachments
(1 file, 2 obsolete files)
25.87 KB,
patch
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → anujagarwal464
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8493894 -
Flags: feedback?(continuation)
Reporter | ||
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
Fixed guard. Try : https://tbpl.mozilla.org/?tree=Try&rev=3a3d372ff6a5
Attachment #8493894 -
Attachment is obsolete: true
Attachment #8493915 -
Flags: review?(continuation)
Reporter | ||
Updated•10 years ago
|
Attachment #8493915 -
Flags: review?(continuation) → review?(nfroyd)
Comment 4•10 years ago
|
||
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+
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=b51ee5bbea4e
Attachment #8493915 -
Attachment is obsolete: true
Reporter | ||
Comment 7•10 years ago
|
||
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
Reporter | ||
Comment 8•10 years ago
|
||
I'll post about this on dev.platform when it merges, just so people know it is an option.
Flags: needinfo?(continuation)
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/39f6aa4ae636
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Reporter | ||
Comment 10•10 years ago
|
||
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.
Description
•