1335780 - Make Maybe<T>::emplace() work when T is const

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jonathan Watt [:jwatt]]

Right now Maybe<T>::emplace() doesn't work if T is const because the implementation of |new| can't assign the const pointer returned by mStorage.addr() to a non-const void*. |emplace| should really work in this case though since in principle we're initializing uninitialized memory.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Make Maybe<T>::emplace() work when T is const

I'll attach tests shortly.

Comment 2

[Jeff Walden [:Waldo]]

This smells funny.  There are various limitations in the C++ spec surrounding const objects regarding how they can/can't be mutated, and this has the "look" of being something that's likely undefined behavior in C++.  It may well be the case that Maybe<const T> should be formally prohibited as a thing.  Some spec-wonkery seems necessary to verify that doing this is actually correct; my gut says it's not going to be, but I can't back that up with citation to spec yet.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> This smells funny.  There are various limitations in the C++ spec
> surrounding const objects regarding how they can/can't be mutated, and this
> has the "look" of being something that's likely undefined behavior in C++. 
> It may well be the case that Maybe<const T> should be formally prohibited as
> a thing.  Some spec-wonkery seems necessary to verify that doing this is
> actually correct; my gut says it's not going to be, but I can't back that up
> with citation to spec yet.

Part of the problem with this is that our documentation says that emplace() is the way to initialize Maybe<const T>:

http://dxr.mozilla.org/mozilla-central/source/mfbt/Maybe.h#473

and IRC discussion with jwatt indicates that Maybe<const T> is basically unusable, which seems very unfortunate.  AFAICT, std::optional and boost::optional don't place non-const limitations on the contained type, and we shouldn't either, so far as we are able.  At least one of the draft proposals for std::optional explicitly contains optional<const T> examples:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3672.html#overview.usecases

That section was removed in a later draft, but I can't find any language in that draft that would prohibit optional<const T>:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3793.html#rationale.requirements

std::optional contains optional(in_place_t&, Args&&..) constructors that I think are there to let you declare Maybe objects thus:

  Maybe<const T> obj(in_place, ...);

which are an avenue worth investigating as well.  But wasn't sure how jwatt felt about implementing code like that, and if emplace() worked with a small tweak, that seemed much more reasonable in any event.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8832654 [details] [diff] [review]
part 1 - Make Maybe<T>::emplace() work when T is const

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

I verified the below solution works with .emplace() in a hand-written test, FWIW.  r=me with that change.

::: mfbt/Maybe.h
@@ +449,5 @@
>    template<typename... Args>
>    void emplace(Args&&... aArgs)
>    {
>      MOZ_ASSERT(!mIsSome);
> +    ::new (const_cast<typename RemoveCV<T>::Type *>(mStorage.addr())) T(Forward<Args>(aArgs)...);

I think instead of abusing const_cast here, we should just be more honest about what kind of type our storage contains:

@@ -85,7 +85,8 @@ template<class T>
 class Maybe
 {
   bool mIsSome;
-  AlignedStorage2<T> mStorage;
+  typedef typename RemoveCV<T>::Type StorageType;
+  AlignedStorage2<StorageType> mStorage;
 
 public:
   typedef T ValueType;

and then all the const-ness comes from public member functions on Maybe, so maybe that const-scariness Waldo was referring to stops being a concern.

Indeed, looking at GCC's <optional>, I see:

    private:
      // Remove const to avoid prohibition of reusing object storage for
      // const-qualified types in [3.8/9]. This is strictly internal
      // and even optional itself is oblivious to it.
      using _Stored_type = remove_const_t<_Tp>;

so we are on the right track here.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: part 2 - tests

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8833120 [details] [diff] [review]
part 2 - tests

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

Thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a10b506ab805
part 1 - Make Maybe<T>::emplace() work when T is const. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/345fe2dd1dc9
part 2 - Tests for Maybe<const Type>. r=froydnj

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a10b506ab805
https://hg.mozilla.org/mozilla-central/rev/345fe2dd1dc9