Closed
Bug 1246841
Opened 9 years ago
Closed 9 years ago
Allow construction of Variant values using type inference
Categories
(Core :: MFBT, defect)
Core
MFBT
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));
> }
Assignee | ||
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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+
Assignee | ||
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8717274 -
Attachment is obsolete: true
Comment 6•9 years ago
|
||
(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.
Assignee | ||
Comment 7•9 years ago
|
||
(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?
Assignee | ||
Comment 8•9 years ago
|
||
(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.
Assignee | ||
Comment 9•9 years ago
|
||
Implemented the change discussed above.
Assignee | ||
Updated•9 years ago
|
Attachment #8719100 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8901dd8c722185016e3ccc2c4056dedfbf6a033a
Bug 1246841 - Allow construction of Variant values using type inference. r=waldo
Comment 11•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•