1466909 - UniquePtr::operator*() should use AddLvalueReference

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[André Bargull [:anba]]

|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

Comment 1

[André Bargull [:anba]]

Attachment: bug1466909.patch

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?

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[André Bargull [:anba]]

(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 4

[Nathan Froyd [:froydnj]]

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!

Comment 5

[André Bargull [:anba]]

Attachment: bug1466909.patch

Updated patch to include test case, carrying r+ from froydnj.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17b140524c6e
Use AddLvalueReference for UniquePtr's operator*(). r=froydnj

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/17b140524c6e