Make nsCOMPtr constructible&assignable from nullptr

RESOLVED FIXED in Firefox 52

Status

()

Core
XPCOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

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

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
Similarly to bug 1316206, which made RefPtr work with nullptr, we should do the same with nsCOMPtr.

Repeating the expected benefits:
- It would appear as a separate call in stack traces.
- Slight optimization, as there would be no need to check for a non-null pointer that would need an AddRef().
- It will enforce the Mozilla coding style that requires the use of 'nullptr' for pointers in C++.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8809354 [details]
Bug 1316432 - Fix nsCOMPtr constructions&assignments from 0 -

https://reviewboard.mozilla.org/r/91978/#review91960
Attachment #8809354 - Flags: review?(nfroyd) → review+

Comment 6

2 years ago
mozreview-review
Comment on attachment 8809352 [details]
Bug 1316432 - Replace 0's with nullptr in nsCOMPtr.* -

https://reviewboard.mozilla.org/r/91974/#review91962
Attachment #8809352 - Flags: review?(nfroyd) → review+

Comment 7

2 years ago
mozreview-review
Comment on attachment 8809353 [details]
Bug 1316432 - nsCOMPtr construction/assignment from nullptr -

https://reviewboard.mozilla.org/r/91976/#review91964

r=me with or without addressing the comment below.

::: xpcom/glue/nsCOMPtr.h:435
(Diff revision 1)
> +  MOZ_IMPLICIT nsCOMPtr(std::nullptr_t)
> +    : NSCAP_CTOR_BASE(nullptr)

With the `RefPtr` patches, you went with `decltype(nullptr)` for these overloads; was there a particular reason for the switch here?  I see the `::operator==` overloads for `nsCOMPtr` use `decltype(nullptr)`, and I guess I have a slight (like epsilon-slight) preference for using that instead of `std::nullptr_t`.
Attachment #8809353 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8809353 [details]
Bug 1316432 - nsCOMPtr construction/assignment from nullptr -

https://reviewboard.mozilla.org/r/91976/#review91964

> With the `RefPtr` patches, you went with `decltype(nullptr)` for these overloads; was there a particular reason for the switch here?  I see the `::operator==` overloads for `nsCOMPtr` use `decltype(nullptr)`, and I guess I have a slight (like epsilon-slight) preference for using that instead of `std::nullptr_t`.

Oh dear, I thought there was no 'decltype(nullptr)' in nsCOMPtr.h, that's why I used the more std:: route!
Ok I'll revert to decltype to be more consistent in the file and with RefPtr.

Should we make this a Coding Style rule, so it's the same everywhere?
Doing a quick foxsearch, at the moment the score is: nullptr_t 45 vs decltype(nullptr) 79
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 years ago
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/09a8134e26ea
Replace 0's with nullptr in nsCOMPtr.* - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/376b38e78518
nsCOMPtr construction/assignment from nullptr - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/8d3dcf1db881
Fix nsCOMPtr constructions&assignments from 0 - r=froydnj

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/09a8134e26ea
https://hg.mozilla.org/mozilla-central/rev/376b38e78518
https://hg.mozilla.org/mozilla-central/rev/8d3dcf1db881
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

2 years ago
Depends on: 1317368
You need to log in before you can comment on or make changes to this bug.