Closed Bug 1466909 Opened 2 years ago Closed 2 years ago

UniquePtr::operator*() should use AddLvalueReference

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(1 file, 1 obsolete file)

|std::unique_ptr::operator*()| uses |std::add_lvalue_reference<T>::type| for its return type [1], whereas |UniquePtr::operator*()| uses a plain |T&| [2].

Use case: UniqueFreePtr<void> doesn't compile when |T&| is used.


[1] https://en.cppreference.com/w/cpp/memory/unique_ptr/operator*
[2] https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/mfbt/UniquePtr.h#317
Blocks: 1467438
Attached patch bug1466909.patch (obsolete) — Splinter Review
For bug 1467438 I'll need |UniquePtr<void, JS::FreePolicy>| and without this change it won't compile because |void&| is not well formed.

Do you think this change requires a test case?
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8984117 - Flags: review?(nfroyd)
Comment on attachment 8984117 [details] [diff] [review]
bug1466909.patch

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

I'm surprised this compiles, since the *get() inside operator*() for UniquePtr<void> would still be dereferencing void*, which doesn't work.

I think you need to also uninline operator*(), so that the method doesn't have to be instantiated right away.  Or is there some weird way that this works for you?

As long as we're going back and forth, we might as well get a small test in.
Attachment #8984117 - Flags: review?(nfroyd) → review-
(In reply to Nathan Froyd [:froydnj] from comment #2)
> I think you need to also uninline operator*(), so that the method doesn't
> have to be instantiated right away.  Or is there some weird way that this
> works for you?

It appears to work even if the method is still inlined in the UniquePtr class definition: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c9a9dd851038c48a6512d0b22c26b406c9d997b


When the operator*() was only called, a warning was issued:
---
In file included from /home/andre/hg/mozilla-inbound/mfbt/tests/TestUniquePtr.cpp:11:
/home/andre/hg/mozilla-inbound/obj-x86_64-pc-linux-gnu-debug/dist/include/mozilla/UniquePtr.h:317:67: warning: ISO C++ does not allow indirection on operand of type 'mozilla::UniquePtr<void,
      mozilla::detail::FreePolicy<void> >::Pointer' (aka 'void *') [-Wvoid-ptr-dereference]
  typename AddLvalueReference<T>::Type operator*() const { return *get(); }
                                                                  ^~~~~~
/home/andre/hg/mozilla-inbound/mfbt/tests/TestUniquePtr.cpp:593:3: note: in instantiation of member function 'mozilla::UniquePtr<void, mozilla::detail::FreePolicy<void> >::operator*' requested here
  *p2;
---


And only when the result of operator*() was actually assigned to a variable, an error was thrown:
---
/home/andre/hg/mozilla-inbound/mfbt/tests/TestUniquePtr.cpp:593:8: error: variable has incomplete type 'void'
  auto v = *p2;
       ^
In file included from /home/andre/hg/mozilla-inbound/mfbt/tests/TestUniquePtr.cpp:11:
/home/andre/hg/mozilla-inbound/obj-x86_64-pc-linux-gnu-debug/dist/include/mozilla/UniquePtr.h:317:67: warning: ISO C++ does not allow indirection on operand of type 'mozilla::UniquePtr<void,
      mozilla::detail::FreePolicy<void> >::Pointer' (aka 'void *') [-Wvoid-ptr-dereference]
  typename AddLvalueReference<T>::Type operator*() const { return *get(); }
                                                                  ^~~~~~
/home/andre/hg/mozilla-inbound/mfbt/tests/TestUniquePtr.cpp:593:12: note: in instantiation of member function 'mozilla::UniquePtr<void, mozilla::detail::FreePolicy<void> >::operator*' requested here
  auto v = *p2;
---
Comment on attachment 8984117 [details] [diff] [review]
bug1466909.patch

Huh, OK; my mental model of templates needs updating somehow.  r=me on the patch as pushed to try, with the tests added.  Thank you!
Attachment #8984117 - Flags: review- → review+
Attached patch bug1466909.patchSplinter Review
Updated patch to include test case, carrying r+ from froydnj.
Attachment #8984117 - Attachment is obsolete: true
Attachment #8984281 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17b140524c6e
Use AddLvalueReference for UniquePtr's operator*(). r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/17b140524c6e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.