Closed Bug 1410760 Opened 7 years ago Closed 4 years ago

NotNull's comment incorrectly claims that NotNull<UniquePtr<T>> "doesn't work"

Categories

(Core :: MFBT, defect)

Product:
Core
Component:
MFBT
Version:
58 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox86 --- fixed

People

(Reporter: mozbugz, Assigned: sg)

References

Details

Attachments

(1 file)

Bug 1410760 - Fix comment on NotNull<UniquePtr<T>>. r=gerald
(deleted), text/x-phabricator-request
Details
Spawned from bug 1410252 comment 5. The comment just above the class definition is: [1] > // NotNull currently doesn't work with UniquePtr. See > // https://github.com/Microsoft/GSL/issues/89 for some discussion. Reading that discussion, the main issue is that std::unique_ptr cannot be copy-constructed, but the official gsl::not_null can *only* be copy-constructed, so not_null<unique_ptr<T>> will always produce compiler errors. However, mozilla::NotNull has defaulted move constructor and assignment operator [2], so move-only types should be usable with it. As proof, the test added in bug 1410252 creates a NotNull<UniquePtr<int>> without issues. I think we should modify the comment (to indicate that move-only pointer types like UniquePtr work, unlike in GSL), and add a few tests to prove that it fully works as expected. Maybe for completeness, the copy&move special members should be EnableIf'd based on the inner pointer type capabilities? [1] http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/mfbt/NotNull.h#98-99 [2] http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/mfbt/NotNull.h#125-129
As part of this, we could tackle the following tweak to MakeNotNull: (In reply to David Baron :dbaron: from bug 1410252 comment #12) > "Pointee" seems like an odd term to my ears. Perhaps "Target" would be > better? (I don't see "pointee" in > https://en.wikipedia.org/wiki/Pointer_(computer_programming) and I see a > *few* occurrences of "target", but maybe there's something better... maybe a > term related to type polymorphism?) I'd personally be happy with "Target" or "PointedToType".
A quick search shows almost 300 "pointee"s in the tree, including in mfbt and xpcom, and in external code. It doesn't prove it's the correct word, but it shows it's been used elsewhere without trouble. Maybe we should start a war on m.d.platform to decide!? ;-)
> Maybe we should start a war on m.d.platform to decide!? ;-) Let's not :) "Pointee" also sounds slightly strange to me, but its meaning is clearer than any alternative I can think of, so IMO it doesn't need changing.

Just discussed about NotNull and UniquePtr, this made me find this old bug! 😄
Some extra thoughts:

And I also found I actually tested NotNull<UniquePtr<>> there:
https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/mfbt/tests/TestNotNull.cpp#336
But it's a bare test, I think more needs to be done to officially support NotNull<UniquePtr<>>.

In particular, how to deal with moving from a NotNull<UniquePtr<>>? Because it's a unique pointer, we need to reset the moved-from object to null, which would break the NotNull invariant. But then a moved-from object should not be used anymore, except to destroy it and maybe re-assign it.
At the moment I'm thinking that NotNull<UniquePtr<>> may need a specialization, or some extra asserts, to make sure that a moved-from object (that contains a nullptr) is not queried until destruction or re-assignment.

A specialization could redefine get() to return a NotNull<>.

And what about UniquePtr<NotNull<>>, is that possible, and should it behave the same way?

Bug 1634003 added some things to make this more usable, in particular a MovingNotNull class template, and it actually added a test with UniquePtr: https://searchfox.org/mozilla-central/rev/8d722de75886d6bffc116772a1db8854e34ee6a7/mfbt/tests/TestNotNull.cpp#360-377

But the comment is still there, and should be removed.

I am not sure if this addresses all your concerns/questions?

And what about UniquePtr<NotNull<>>, is that possible,

I guess it's possible as there are no particular constraints on the pointee type of a UniquePtr, but is that useful?

and should it behave the same way?

I guess not. It would be a possible null unique pointer to a non-null pointer...

Flags: needinfo?(gsquelart)
Depends on: 1634003

Sorry, I didn't notice the NI until now.

Yes, bug 1634003 helps, thank you.

I can't remember the exact problem with UniquePtr<NotNull<...>> though, it now feels like it "should just work", but it's probably not very useful anyway -- I guess it could happen through some generic code.

Anyway, happy for this bug to focus again on fixing the old comment.

Flags: needinfo?(gsquelart)
Assignee: nobody → sgiesecke
Status: NEW → ASSIGNED
Summary: NotNull's comment incorrectly claim that NotNull<UniquePtr<T>> "doesn't work" → NotNull's comment incorrectly claims that NotNull<UniquePtr<T>> "doesn't work"
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/93785e980e88 Fix comment on NotNull<UniquePtr<T>>. r=gerald DONTBUILD

https://hg.mozilla.org/mozilla-central/rev/93785e980e88

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox86: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: