Closed Bug 1335780 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Blocks: 1058040
I'll attach tests shortly.
Attachment #8832654 - Flags: review?(nfroyd)
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.
(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 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.
Attachment #8832654 - Flags: review?(nfroyd) → review+
Attached patch part 2 - testsSplinter Review
Attachment #8833120 - Flags: review?(nfroyd)
Comment on attachment 8833120 [details] [diff] [review]
part 2 - tests

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

Thanks!
Attachment #8833120 - Flags: review?(nfroyd) → review+
Pushed by jwatt@jwatt.org:
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
Blocks: 1345840
You need to log in before you can comment on or make changes to this bug.