Closed Bug 912299 Opened 6 years ago Closed 6 years ago

Make RefCounted's refcount field mutable to allow nsRefPtr<const T>

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: seth, Assigned: seth)

Details

Attachments

(1 file, 3 obsolete files)

We should be advocating the use of const everywhere possible because it produces code that is easier to reason about. Unfortunately, time and again I've had to remove const from whole swathes of my code because in some deeply nested place I needed to use nsRefPtr on a const type. We should fix this by making the refcount mutable and marking the appropriate methods (at least AddRef and Release) as const.
Assignee: nobody → seth
Here's the proposed patch. With these changes, nsRefPtr<const T> compiles just fine.
Attachment #799207 - Flags: review?(jwalden+bmo)
Silly me, realized that one of the lines in the previous patch could be accomplished in a simpler way. New, cleaner patch attached.
Attachment #799226 - Flags: review?(jwalden+bmo)
Attachment #799207 - Attachment is obsolete: true
Attachment #799207 - Flags: review?(jwalden+bmo)
I'm not convinced. You *are* changing the memory you point at, after all.
Er.  Hit the "Save Changes" button too soon.  I view the chromium discussion as suggesting that maybe we should do this -- or at least that nobody there was too adamantly opposed, and nobody raised super-compelling reasons not to do this.  I haven't found any further design discussions in other communities (except some sort of boost::refcounted hit, that also had mutable refcounts, but without discussion or justification that I could find).  A little more looking around is probably worthwhile.
Why exactly wouldn't we do this? This is exactly the kind of scenario that the 'mutable' keyword is for - situations where the object contains metadata that does not logically affect its value. Another example would be memoization of function return values. C++ 'const' is about logical constness, not bitwise constness.

By choosing not to do this, we'd be throwing the baby out with the bathwater. There are a lot of cases in which we can't use const in our interfaces right now because of this issue. I'd rather see const used much more widely, and that means making it possible to pass around nsRefPtr's to const objects. I have personally had to remove _many_ const annotations because of this issue, which other reference counting libraries don't have.
I say +1 for |mutable| refcounts; I tend to agree with Seth's line of thinking.
Comment on attachment 799226 [details] [diff] [review]
Make RefCounted's refcount field mutable.

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

Searching a bit further, I think the last sentence of <http://stackoverflow.com/a/1516646/115698> is perhaps the most convincing argument here: "the object just happens to be a convenient place to store this semi-unrelated data [the reference count]".  I buy that; let's run with this.

::: mfbt/RefPtr.h
@@ +84,5 @@
>      }
>  
>      // Compatibility with wtf::RefPtr.
>      void ref() { AddRef(); }
>      void deref() { Release(); }

These two methods need const as well.  I'm slightly surprised this compiles, actually.
Attachment #799226 - Flags: review?(jwalden+bmo) → review+
\o/ Thanks for the review, Jeff! And there was much rejoicing!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> >      // Compatibility with wtf::RefPtr.
> >      void ref() { AddRef(); }
> >      void deref() { Release(); }
> 
> These two methods need const as well.  I'm slightly surprised this compiles,
> actually.

Well, you can call const methods from non-const ones!

I'll be happy to change those if you want, but let me just bounce my reasoning for leaving them as-is off you and see if you agree or disagree. I figure this is support for interop with WTF and code that uses it, which is foreign code that we didn't write. In that situation it makes sense to me to maintain the foreign semantics and forbid const ref() and deref() since wtf::RefPtr does not allow it. (AFAICT.)

What do you think? If you still want me to change them I'll go for it.
(In reply to Seth Fowler [:seth] from comment #9)
> Well, you can call const methods from non-const ones!

Er, right.  :-)

> I'll be happy to change those if you want, but let me just bounce my
> reasoning for leaving them as-is off you and see if you agree or disagree. I
> figure this is support for interop with WTF and code that uses it, which is
> foreign code that we didn't write. In that situation it makes sense to me to
> maintain the foreign semantics and forbid const ref() and deref() since
> wtf::RefPtr does not allow it. (AFAICT.)

That seems pretty sensible, actually.
Followup to fix a build issue on Android.
Hopefully this will fix the Android bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/39bcc4b7efc8
Backed 'em both out in http://hg.mozilla.org/integration/mozilla-inbound/rev/98656b5a9e6b since it wasn't enough to please Android.
Since I got backed out anyway due to Android build failures, folded the two patches together.

It appears there was one more 'const' needed in ElfLoader.h. I've also added that in this patch.
Attachment #799226 - Attachment is obsolete: true
Attachment #800579 - Attachment is obsolete: true
Let's see if we can't get a green build on Android:

https://tbpl.mozilla.org/?tree=Try&rev=c06d072b6d8a
https://hg.mozilla.org/mozilla-central/rev/e2796e169e28
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.