Closed Bug 772987 Opened 8 years ago Closed 8 years ago

Add Static{Auto,Ref}Ptr, like ns{Auto,Ref}Ptr, but without the static initializer

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(3 files)

It's silly that, when we want static variables, we have to choose between startup speed (raw pointers) and safety (using static ns{Auto,Ref}Ptr, which creates an additional static initializer).

I have a patch to add clones of ns{Auto,Ref}Ptr which don't add static initializers.
Assignee: nobody → justin.lebar+bug
Attachment #641158 - Flags: review?(mh+mozilla)
This is mostly hal code, so over to Mounir.
Attachment #641159 - Flags: review?(mounir)
Actually, I should probably hold off on the reviews until after I get try results back, because I have a hunch something isn't going to compile somewhere.
Attachment #641158 - Flags: review?(mh+mozilla)
Attachment #641159 - Flags: review?(mounir)
Comment on attachment 641158 [details] [diff] [review]
Part 1: Add Static{Auto,Ref}Ptr.

Can you please file a static analysis bug to make us actually check that these classes are only used in the global scope?  I wouldn't trust people with this kind of power even for a second!  ;-)
> I wouldn't trust people with this kind of power even for a second!  ;-)

There's a dynamic (debug-time) assertion that mRawPtr is 0, during the constructor.  So hopefully that will catch some cases.

Static analysis would be great, though.
(In reply to comment #7)
> > I wouldn't trust people with this kind of power even for a second!  ;-)
> 
> There's a dynamic (debug-time) assertion that mRawPtr is 0, during the
> constructor.  So hopefully that will catch some cases.

Ah, hmm, that should take care of a whole bunch of cases, yes.  But I mean, once we get NS_STACK_CLASS working (which is close if I understand things correctly), this should be easy.  :-)
Blocks: 773014
Comment on attachment 641158 [details] [diff] [review]
Part 1: Add Static{Auto,Ref}Ptr.

try is pretty horked, with no recovery in sight, so I guess I'll just use m-i as my tryserver.
Attachment #641158 - Flags: review?(mh+mozilla)
Attachment #641159 - Flags: review?(mounir)
Comment on attachment 641165 [details] [diff] [review]
Part 3: Update comment in ClearOnShutdown.

I'd normally ask Waldo to review this comment change, but he's busy.
Attachment #641165 - Flags: review?(mh+mozilla)
Attachment #641159 - Flags: review?(mounir) → review+
Attachment #641158 - Flags: review?(mh+mozilla)
Attachment #641158 - Flags: review?(benjamin)
Attachment #641158 - Flags: feedback+
Attachment #641165 - Flags: review?(mh+mozilla) → review+
You might be able to ~export

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsTraceRefcntImpl.cpp#133

and use it to assert that these helpers are *only* constructed/destructed from ~static contexts.
Seems like that would only work in builds with --enable-trace-malloc (which isn't the default with debug builds?  Or is it?).  In any case, I could certainly do something similar.  The trick is hearing that we're starting up very early in the game, and hearing that we're shutting down very late in the game.
Wouldn't it be less maintenance burden to make these base classes of nsAutoPtr / nsRefPtr and move all methods except for the destructor to them?  Then there'd be no duplication of code, they'd provide the same API, and any future additions would automatically apply to both.
er, destructor and constructor.  (You'd also need to add a second protected constructor with a funky signature for the derived class to call, to avoid the assert.)
(In reply to David Baron [:dbaron] from comment #13)
> Wouldn't it be less maintenance burden to make these base classes of
> nsAutoPtr / nsRefPtr and move all methods except for the destructor to them?
> Then there'd be no duplication of code, they'd provide the same API, and any
> future additions would automatically apply to both.

I considered that, but I rejected it because I'd still have to duplicate a bunch of operators on nsAutoPtr which return nsAutoPtr&, and fix similar problems.  (Maybe there's a way around these using C++-fu that I do not yet have.)

I'm not really bothered that you can't do getter_AddRefs() on a StaticRefPtr, personally.
Attachment #641158 - Flags: review?(benjamin) → review+
Try run for 9a78335b8413 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9a78335b8413
Results (out of 17 total builds):
    exception: 17
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-9a78335b8413
Maybe this should have gone into mfbt...
Well, the mfbt RefPtr is not compatible with XPCOM objects...
(In reply to Justin Lebar [:jlebar] from comment #20)
> Well, the mfbt RefPtr is not compatible with XPCOM objects...

That's news to me... why?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> (In reply to Justin Lebar [:jlebar] from comment #20)
> > Well, the mfbt RefPtr is not compatible with XPCOM objects...
> 
> That's news to me... why?

Actually, I'm totally incorrect.
Yes, in fact, thanks to Bas it was made MSCOM-compatible at the cost of some unsafety.
I'd expect compilers to eliminate static initializers that just set members to zero.  Was that not happening?
(In reply to Jesse Ruderman from comment #25)
> I'd expect compilers to eliminate static initializers that just set members
> to zero.  Was that not happening?

GCC, at least, does not do this.  (Long-standing PR.)
There's also the problem of non-trivial static /destructors/, which nsRefPtr does have.  And I vaguely recall something awful like the presence of a non-trivial static destructor requires the presence of a static constructor.
(In reply to Justin Lebar [:jlebar] from comment #27)
> There's also the problem of non-trivial static /destructors/, which nsRefPtr
> does have.  And I vaguely recall something awful like the presence of a
> non-trivial static destructor requires the presence of a static constructor.

Indeed, static destructors are registered with __cxa_atexit in a static constructor. Note that this one applies to MSVC too, contrary to the other case mentioned.
You need to log in before you can comment on or make changes to this bug.