can't hold a nsRefPtr to a class without empty ctor

RESOLVED FIXED

Status

()

RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Right now you'll get a warning on mac and compileerror on some ports if you do
the following:

class A {
  A(int i);

  Addref();
  Release();

  int GetI();
}


    nsRefPtr<A> a = ...;
    a->GetI();                <- warns or fails to compiles


The reason is that we're returning nsRefPtr::operator-> returns an
nsDeriviedSafe<A>, however that class can not be instantiated because it uses a
default ctor but inherits a class with only non-empty ctors (which makes the
default ctor not compile).

So far I've worked around this by adding a non-implemented empty ctor to A, but
that's not too neat.

A better solution is to add a non-implemented ctor to nsDeriviedSafe. This
should work fine since we have other non-implemented non-virtual functions in
that class.

Proposed patch comming up
Created attachment 125822 [details] [diff] [review]
Patch to nsDeriviedSafe

This patch fixes the problem in nsDerivedSafe
Created attachment 125823 [details] [diff] [review]
Patch to remove workarounds

This patch removes the workarounds that I know of in the tree. I'll check this
in together with attachment 125822 [details] [diff] [review] to make sure that it works on all platforms.
Attachment #125822 - Flags: superreview?(dbaron)
Attachment #125822 - Flags: review?(jaggernaut)
Attachment #125823 - Flags: superreview?(peterv)
Attachment #125823 - Flags: review?(axel)
Status: NEW → ASSIGNED
Comment on attachment 125822 [details] [diff] [review]
Patch to nsDeriviedSafe

This looks good to me.	You might want to check with scc as well if you can
reach him.
Attachment #125822 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 125822 [details] [diff] [review]
Patch to nsDeriviedSafe

Actually, there's a typo in the first line here: (Derivied instead of Derived):
>+          This ctor exists to avoid compile errors and warnings about nsDeriviedSafe using the
>+          default ctor but inheriting classes without an empty ctor. See bug 209667.

Also, probably "inheriting from" is better than "inheriting".

Comment 5

16 years ago
Comment on attachment 125823 [details] [diff] [review]
Patch to remove workarounds

I wonder if you could make somebody from ports test this before checking it in.
Attachment #125823 - Flags: review?(axel) → review+
Attachment #125823 - Flags: superreview?(peterv) → superreview+

Updated

16 years ago
Blocks: 209506

Comment 6

16 years ago
I don't quite understand what's going on here... operator-> returns a pointer
(nsDerivedSafe<A>*), the default constructor for nsDerivedSafe<A> shouldn't have
to be called.

Unless the compiler isn't following the rule that it shouldn't complain about
stuff in templates until that part of the template is actually used.

Comment 7

16 years ago
Hmmm, though I'm not sure if that rule also applies to constructors.
Right, the constructor is never called, but the compiler is warning based on an
attempt to construct that constructor.  If we say that we implement the
constructor (which is impossible to do, in general, since we don't know how to
construct the base class), then the compiler won't care unless it actually needs
that implementation.

Comment 9

16 years ago
Comment on attachment 125822 [details] [diff] [review]
Patch to nsDeriviedSafe

Hmm, okay.

Shouldn't that be in a |private| section though?

r=jag if you change it to private or explain to me why it should be protected.
I'm afraid that if we put it in the private-section we will get warnings about
the class only having private ctors and no friends and therefor is impossible to
instantiate.
Comment on attachment 125822 [details] [diff] [review]
Patch to nsDeriviedSafe

r=jag
Attachment #125822 - Flags: review?(jaggernaut) → review+
checked in without causing any redness, so all is well
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
IIRC, there's workarounds for this bug in the editor code too, not sure where I
saw that tho...
You need to log in before you can comment on or make changes to this bug.