Closed Bug 1246841 Opened 4 years ago Closed 4 years ago

Allow construction of Variant values using type inference

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 2 obsolete files)

Variant types can become quite verbose (e.g. Variant<const char*, Vector<Pair<HashKey, Value>>, RefPtr<DOMWindow>>) and right now constructing a Variant value requires specifying the entire type. Typedefs can alleviate this problem but they can often be difficult to name with clarity and even confuse the reader in some situations, since it's not obvious that a Variant type is being used.

It'd be preferable to have a lightweight, terse, readable alternative to specifying the entire type or using typedefs. This would reduce the cognitive load of Variant types and make them easier to use in more situations.

In this bug we'll implement AsVariant(), a helper function that constructs a value of an appropriate Variant type from a value of one of its subtypes. AsVariant() is used like the Some() function that Maybe provides:

> Variant<X, Y, Z> foo()
> {
>   return AsVariant(X(100));
> }
This patch implements AsVariant(). The implementation is straightforward:
AsVariant() returns a temporary type, AsVariantTemporary, which stores a
temporary copy of the value. An implicit conversion allows appropriate Variant
types to be constructed from AsVariantTemporary.

My first implementation of this patch stored a reference in AsVariantTemporary
to ensure that this was totally free, but unfortunately this is dangerous in
combination with lambda return type inference, because the return type will be
inferred as AsVariantTemporary and the reference will usually no longer be valid
outside of the body of the lambda. Even though copying means that AsVariant() is
only appropriate for use with primitive or very small types, that still covers
many if not most use cases, and the additional safety is far preferable.
Attachment #8717274 - Flags: review?(jwalden+bmo)
Blocks: 1246851
Comment on attachment 8717274 [details] [diff] [review]
Allow construction of Variant values using type inference.

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

::: mfbt/Variant.h
@@ +255,5 @@
> + */
> +template <typename T>
> +struct AsVariantTemporary
> +{
> +  AsVariantTemporary(const T& aValue)

There's no reason this can't handle movable types safely, IMO:

  template<typename U>
  explicit AsVariantTemporary(U&& aValue)
    : mValue(Forward<U>(aValue))
  {}

Also, to avoid people keeping an AsVariant() around too long, probably worth adding

  AsVariantTemporary() = delete;
  AsVariantTemporary(const AsVariantTemporary&) = delete;
  void operator=(const AsVariantTemporary&) = delete;

@@ +452,5 @@
>      new (this) Variant(aRhs);
>      return *this;
>    }
>  
> +  /** Copy assignment from AsVariant(). */

Why is this necessary?  It seems to me you only want to support the pattern of assigning AsVariant(...) directly into a Variant, or by way of a return statement.  This method no longer makes that possible.  Please explain why this method can't/shouldn't be removed.

@@ +562,5 @@
> +/*
> + * AsVariant() is used to construct a Variant<T,...> value containing the
> + * provided T value using type inference. It can be used to construct Variant
> + * values in expressions or return them from functions without specifying the
> + * entire Variant type.

Add "Don't use this in other situations" or somesuch, please.
Attachment #8717274 - Flags: review?(jwalden+bmo) → review+
Thanks for the review!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Why is this necessary?  It seems to me you only want to support the pattern
> of assigning AsVariant(...) directly into a Variant, or by way of a return
> statement.  This method no longer makes that possible.  Please explain why
> this method can't/shouldn't be removed.

I don't understand this comment. Why wouldn't we want to support:

> Variant<A, B, C> foo = AsVariant(A());
> ...
> foo = AsVariant(B());

even if B is not movable?

It's quite possible I'm missing something.
An updated version of the patch which makes all the changes above except for
removing the "Copy assignment from AsVariant()" code.

Note that:

- We do need to copy AsVariantTemporary objects sometimes, so I didn't delete
  the copy constructor.

- I realized that we should remove const qualifiers and references from the type
  of mValue, so I did so in this version of the patch.
Attachment #8717274 - Attachment is obsolete: true
(In reply to Seth Fowler [:seth] [:s2h] from comment #3)
> I don't understand this comment. Why wouldn't we want to support:
> 
> > Variant<A, B, C> foo = AsVariant(A());
> > ...
> > foo = AsVariant(B());
> 
> even if B is not movable?
> 
> It's quite possible I'm missing something.

AsVariant() returns a temporary value, so |foo = AsVariant(...)| will call |foo|'s move assignment operator.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> AsVariant() returns a temporary value, so |foo = AsVariant(...)| will call
> |foo|'s move assignment operator.

Yes, but what if B is not movable? In that case the copy assignment operator will be called, because AsVariantTemporary will *not* have an implicitly defined move assignment operator, because it has a member of type B that is not movable. We must define the copy assignment operator to handle that case. Or am I missing something?
(In reply to Seth Fowler [:seth] [:s2h] from comment #7)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> > AsVariant() returns a temporary value, so |foo = AsVariant(...)| will call
> > |foo|'s move assignment operator.
> 
> Yes, but what if B is not movable? In that case the copy assignment operator
> will be called, because AsVariantTemporary will *not* have an implicitly
> defined move assignment operator, because it has a member of type B that is
> not movable. We must define the copy assignment operator to handle that
> case. Or am I missing something?

Wait, hmm. Nevermind. That would apply if assigning one AsVariantTemporary to another, but not when assigning an AsVariantTemporary to a Variant, wouldn't it? My mistake.
Implemented the change discussed above.
Attachment #8719100 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/8901dd8c722185016e3ccc2c4056dedfbf6a033a
Bug 1246841 - Allow construction of Variant values using type inference. r=waldo
https://hg.mozilla.org/mozilla-central/rev/8901dd8c7221
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.