Add RefPtr construction&assignment from nullptr

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

49 Branch
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

3 years ago
There is no specific constructor/assignment operator taking 'decltype(nullptr)', aka std::nullptr_t.

I think it would be useful for a few reasons:
- It would appear as a separate call in stack traces -- my main reason for this bug, as I wanted to be sure whether some crashes are coming from resetting a RefPtr.
- Slight optimization, as there would be no need to check for a non-null pointer that would need an AddRef().
- It will prevent the use of '0' when constructing/assigning-to a RefPtr (because the compiler would find it ambiguous to choose between RefPtr(nullptr_t) and RefPtr(T*)), which will enforce the Mozilla coding style that requires the use of 'nullptr' for pointers in C++.

Note: I won't use 'std::nullptr_t' in my patches to stay consistent with the existing use of 'decltype(nullptr)' in RefPtr.h. Another bug could take care of that if we wanted to.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

3 years ago
mozreview-review
Comment on attachment 8808885 [details]
Bug 1316206 - Make RefPtr(decltype(nullptr)) MOZ_IMPLICIT -

https://reviewboard.mozilla.org/r/91616/#review91484

::: xpcom/glue/nsBaseHashtable.h:111
(Diff revision 1)
>     */
>    UserDataType Get(KeyType aKey) const
>    {
>      EntryType* ent = this->GetEntry(aKey);
>      if (!ent) {
> -      return 0;
> +      return UserDataType{};

I think this change is much more reasonable. Take [1] for example, ```FunctionInfo``` did not accept the conversion with 0, fortunately we don't use this Get function so the compiler did not generate this code or it will get compile error.


[1]
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/storage/mozStorageConnection.h#354
Comment hidden (mozreview-request)
Assignee

Comment 7

3 years ago
Thank you James.

This enticed me to rewrite the comment for that 'Get(KeyType)' function, to explain more precisely what would happen for different UserDataType's.

Comment 8

3 years ago
mozreview-review
Comment on attachment 8808883 [details]
Bug 1316206 - RefPtr construction/assignment from nullptr -

https://reviewboard.mozilla.org/r/91612/#review91552

r=me; I'm a bit ambivalent about the below change, though.  It's possible I'm missing some rationale for having a separate function instead of just using `assign_assuming_AddRef`, though?

::: mfbt/RefPtr.h:69
(Diff revision 1)
> +  void
> +  assign_nullptr()

I don't think this function is really necessary; `assign_assuming_AddRef` works just as well, even though it's a little odd to see `assign_assuming_AddRef(nullptr)`.
Attachment #8808883 - Flags: review?(nfroyd) → review+

Comment 9

3 years ago
mozreview-review
Comment on attachment 8808884 [details]
Bug 1316206 - Fix RefPtr assignments from 0 -

https://reviewboard.mozilla.org/r/91614/#review91556

Thank you!
Attachment #8808884 - Flags: review?(nfroyd) → review+

Comment 10

3 years ago
mozreview-review
Comment on attachment 8808885 [details]
Bug 1316206 - Make RefPtr(decltype(nullptr)) MOZ_IMPLICIT -

https://reviewboard.mozilla.org/r/91616/#review91578

I agree that implicit `RefPtr` construction from `nullptr` is OK. The `nsBaseHashtable` change is necessary, but unfortunate.

Can you file a followup bug to ensure that `nsCOMPtr` is constructable from `nullptr` as well?

::: xpcom/glue/nsBaseHashtable.h:112
(Diff revision 2)
>     */
>    UserDataType Get(KeyType aKey) const
>    {
>      EntryType* ent = this->GetEntry(aKey);
>      if (!ent) {
> -      return 0;
> +      return UserDataType{};

Ah, this comes in through `nsDataHashtable<T, RefPtr<U>>`, correct?

I'm not a huge fan of this, since we're basically declaring copying is OK in this API.  Then again, the alternate `Get` overload also copies in the same cases that this one would, so we're already being inefficient.  All of these hashtables need an overhaul. :(
Attachment #8808885 - Flags: review?(nfroyd) → review+
Assignee

Comment 11

3 years ago
mozreview-review-reply
Comment on attachment 8808883 [details]
Bug 1316206 - RefPtr construction/assignment from nullptr -

https://reviewboard.mozilla.org/r/91612/#review91552

> I don't think this function is really necessary; `assign_assuming_AddRef` works just as well, even though it's a little odd to see `assign_assuming_AddRef(nullptr)`.

I guess I was hoping the explicit 'mRawPtr = nullptr' assignment could be a little more efficient.
But I think using assign_assuming_AddRef should just end up being the same, as it will be inlined and properly optimized; or it may be even better, as it could use the already-null aNewPtr for the assignment (instead of having to zero a register).
So I'm happy to get rid of this function, thank you for the suggestion.
Assignee

Updated

3 years ago
Blocks: 1316432
Assignee

Comment 12

3 years ago
mozreview-review-reply
Comment on attachment 8808885 [details]
Bug 1316206 - Make RefPtr(decltype(nullptr)) MOZ_IMPLICIT -

https://reviewboard.mozilla.org/r/91616/#review91578

I've opened bug 1316432 to work on nsCOMPtr.

> Ah, this comes in through `nsDataHashtable<T, RefPtr<U>>`, correct?
> 
> I'm not a huge fan of this, since we're basically declaring copying is OK in this API.  Then again, the alternate `Get` overload also copies in the same cases that this one would, so we're already being inefficient.  All of these hashtables need an overhaul. :(

Yes, this is necessary because of cases where 'UserDataType' is a RefPtr, which now cannot be initialized from a literal 0.

As you've noticed, this function (like the other Get) was already returning 'UserDataType' by value anyway. I'm not sure what more I can do here.
We can overhaul APIs in another bug. ;-)

So I'll assume a "ship-it", unless I hear otherwise.
(In reply to Gerald Squelart [:gerald] from comment #12)
> So I'll assume a "ship-it", unless I hear otherwise.

For avoidance of doubt, ship-it was meant here!  Hashtable rewrites are too big to tackle here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

3 years ago
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d78a1d9f6a4a
RefPtr construction/assignment from nullptr - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/9380a01deaf5
Fix RefPtr assignments from 0 - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/30d3890fca98
Make RefPtr(decltype(nullptr)) MOZ_IMPLICIT - r=froydnj
You need to log in before you can comment on or make changes to this bug.