You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2016/03/01 00:14:03 UTC
On implicit construction of objects whose constructor takes multiple parameters
Hi Guys,
Today I was making a code review and I came across the following snippet:
```
metrics.allocated.put(
name,
{strings::join("/", "allocator/allocated", name),
process::defer(self(), [this, name]() {
double result = 0;
foreachvalue (const Slave& slave, this->slaves) {
Option<Value::Scalar> value =
slave.allocated.get<Value::Scalar>(name);
if (value.isSome()) {
result += value.get().value();
}
}
return result;
})});
```
I think by the code here is hard to tell what is happening here. If you look at the declaration of `metrics.allocated` somewhere else you notice that allocated has the following declaration:
```
hashmap<std::string, process::metrics::Gauge> total;
```
And gauge is certainly no container type, nor its constructor takes an `std::initializer_list` as a parameter. In fact its signature is:
```
Gauge::Gauge(const std::string& name, const Deferred<Future<double>()>& f)
```
What is happening here is that brace constructors allows you to infer the type being constructed, which makes the snippet there be equivalent to:
```
metrics.allocated.put(
name,
Gauge(
strings::join("/", "allocator/allocated", name),
process::defer(self(), [this, name]() {
double result = 0;
foreachvalue (const Slave& slave, this->slaves) {
Option<Value::Scalar> value =
slave.allocated.get<Value::Scalar>(name);
if (value.isSome()) {
result += value.get().value();
}
}
return result;
})));
```
What I’m proposing is to only allow this type of construction in a few cases, namely for tuples, pairs and initializer lists where it actually improves readability.
Do you guys have any opinions on the matter?
Re: On implicit construction of objects whose constructor takes
multiple parameters
Posted by Michael Park <mp...@apache.org>.
The only way to find all occurrences of the "undesired" list-initialization
is to mark the constructor as explicit.
If we simply "assume" they are, it's harder for new code to get that right.
1. Current situation
- F(Gauge(a, b, c));
- G({a, b, c});
2. If we simply "assume" they're explicit.
- F(Gauge(a, b, c));
- G(Gauge(a, b, c));
3. New call-site.
- F(Gauge(a, b, c));
- G(Gauge(a, b, c));
- H({a, b, c});
We would need to manually catch (3). All I'm saying is that marking the
constructor as explicit should be a step in (2), which we can leverage to
find other instances anyway, and would prevent (3).
On 2 March 2016 at 10:27, Alexander Rojas <al...@mesosphere.io> wrote:
> That was my first thought, however most of our code doesn’t mark multiple
> argument constructors as `explicit` as this hasn’t been traditionally
> necessary
> so I was thinking on assuming they are until we somehow manage to mark
> all existing constructors as `explicit`.
>
> > On 01 Mar 2016, at 21:41, Michael Park <mp...@apache.org> wrote:
> >
> > What I would propose is to update the style guide to say that a
> constructor
> > be marked *explicit* if implicit conversion or list-initialization is
> > undesired.
> > That is, rather than only marking single-argument constructors as
> > *explicit* which
> > only covers implicit conversions, we should also mark multi-argument
> > constructors as *explicit* if desired, to also cover list-initialization.
> >
> > In this specific case, if you don't want to construct *Gauge* with {x, y,
> > z}, rather than everyone having to remember at every call-site to say
> > *Gauge*, they can be enforced that by marking *Gauge*'s constructor
> > *explicit.*
> >
> > On 1 March 2016 at 11:24, Michael Browning <in...@gmail.com>
> wrote:
> >
> >> I've seen braced initialization in a lot of contexts where the class of
> the
> >> object being initialized doesn't define an initializer_list
> constructor, so
> >> in that sense I think it's an idiomatic usage, and it has the advantage
> of
> >> disallowing narrowing implicit conversions like double to int. On the
> other
> >> hand, widespread use of braced initialization can cause some pain when
> you
> >> inadvertently use it for a type that does take an initializer_list when
> you
> >> meant to refer to a different constructor. I think the the latter case
> is
> >> worth avoiding, and in my opinion your proposal is in line with the
> spirit
> >> of the Mesos style guide as regards type deduction using auto, where it
> >> states:
> >>
> >> The main goal is to increase code readability. This is safely the case
> if
> >> the exact same type omitted on the left is already fully stated on the
> >> right.
> >>
> >>
> >> In your example case with the braced initializer, the type isn't
> stated. I
> >> agree that the explicit construction should be required.
> >>
> >> On Mon, Feb 29, 2016 at 3:14 PM, Alexander Rojas <
> alexander@mesosphere.io>
> >> wrote:
> >>
> >>> Hi Guys,
> >>>
> >>> Today I was making a code review and I came across the following
> snippet:
> >>>
> >>> ```
> >>> metrics.allocated.put(
> >>> name,
> >>> {strings::join("/", "allocator/allocated", name),
> >>> process::defer(self(), [this, name]() {
> >>> double result = 0;
> >>> foreachvalue (const Slave& slave, this->slaves) {
> >>> Option<Value::Scalar> value =
> >>> slave.allocated.get<Value::Scalar>(name);
> >>> if (value.isSome()) {
> >>> result += value.get().value();
> >>> }
> >>> }
> >>> return result;
> >>> })});
> >>> ```
> >>>
> >>> I think by the code here is hard to tell what is happening here. If you
> >>> look at the declaration of `metrics.allocated` somewhere else you
> notice
> >>> that allocated has the following declaration:
> >>>
> >>> ```
> >>> hashmap<std::string, process::metrics::Gauge> total;
> >>> ```
> >>>
> >>> And gauge is certainly no container type, nor its constructor takes an
> >>> `std::initializer_list` as a parameter. In fact its signature is:
> >>>
> >>> ```
> >>> Gauge::Gauge(const std::string& name, const Deferred<Future<double>()>&
> >> f)
> >>> ```
> >>>
> >>> What is happening here is that brace constructors allows you to infer
> the
> >>> type being constructed, which makes the snippet there be equivalent to:
> >>>
> >>> ```
> >>> metrics.allocated.put(
> >>> name,
> >>> Gauge(
> >>> strings::join("/", "allocator/allocated", name),
> >>> process::defer(self(), [this, name]() {
> >>> double result = 0;
> >>> foreachvalue (const Slave& slave, this->slaves) {
> >>> Option<Value::Scalar> value =
> >>> slave.allocated.get<Value::Scalar>(name);
> >>> if (value.isSome()) {
> >>> result += value.get().value();
> >>> }
> >>> }
> >>> return result;
> >>> })));
> >>> ```
> >>>
> >>> What I’m proposing is to only allow this type of construction in a few
> >>> cases, namely for tuples, pairs and initializer lists where it actually
> >>> improves readability.
> >>>
> >>> Do you guys have any opinions on the matter?
> >>
>
>
Re: On implicit construction of objects whose constructor takes multiple parameters
Posted by Alexander Rojas <al...@mesosphere.io>.
That was my first thought, however most of our code doesn’t mark multiple
argument constructors as `explicit` as this hasn’t been traditionally necessary
so I was thinking on assuming they are until we somehow manage to mark
all existing constructors as `explicit`.
> On 01 Mar 2016, at 21:41, Michael Park <mp...@apache.org> wrote:
>
> What I would propose is to update the style guide to say that a constructor
> be marked *explicit* if implicit conversion or list-initialization is
> undesired.
> That is, rather than only marking single-argument constructors as
> *explicit* which
> only covers implicit conversions, we should also mark multi-argument
> constructors as *explicit* if desired, to also cover list-initialization.
>
> In this specific case, if you don't want to construct *Gauge* with {x, y,
> z}, rather than everyone having to remember at every call-site to say
> *Gauge*, they can be enforced that by marking *Gauge*'s constructor
> *explicit.*
>
> On 1 March 2016 at 11:24, Michael Browning <in...@gmail.com> wrote:
>
>> I've seen braced initialization in a lot of contexts where the class of the
>> object being initialized doesn't define an initializer_list constructor, so
>> in that sense I think it's an idiomatic usage, and it has the advantage of
>> disallowing narrowing implicit conversions like double to int. On the other
>> hand, widespread use of braced initialization can cause some pain when you
>> inadvertently use it for a type that does take an initializer_list when you
>> meant to refer to a different constructor. I think the the latter case is
>> worth avoiding, and in my opinion your proposal is in line with the spirit
>> of the Mesos style guide as regards type deduction using auto, where it
>> states:
>>
>> The main goal is to increase code readability. This is safely the case if
>> the exact same type omitted on the left is already fully stated on the
>> right.
>>
>>
>> In your example case with the braced initializer, the type isn't stated. I
>> agree that the explicit construction should be required.
>>
>> On Mon, Feb 29, 2016 at 3:14 PM, Alexander Rojas <al...@mesosphere.io>
>> wrote:
>>
>>> Hi Guys,
>>>
>>> Today I was making a code review and I came across the following snippet:
>>>
>>> ```
>>> metrics.allocated.put(
>>> name,
>>> {strings::join("/", "allocator/allocated", name),
>>> process::defer(self(), [this, name]() {
>>> double result = 0;
>>> foreachvalue (const Slave& slave, this->slaves) {
>>> Option<Value::Scalar> value =
>>> slave.allocated.get<Value::Scalar>(name);
>>> if (value.isSome()) {
>>> result += value.get().value();
>>> }
>>> }
>>> return result;
>>> })});
>>> ```
>>>
>>> I think by the code here is hard to tell what is happening here. If you
>>> look at the declaration of `metrics.allocated` somewhere else you notice
>>> that allocated has the following declaration:
>>>
>>> ```
>>> hashmap<std::string, process::metrics::Gauge> total;
>>> ```
>>>
>>> And gauge is certainly no container type, nor its constructor takes an
>>> `std::initializer_list` as a parameter. In fact its signature is:
>>>
>>> ```
>>> Gauge::Gauge(const std::string& name, const Deferred<Future<double>()>&
>> f)
>>> ```
>>>
>>> What is happening here is that brace constructors allows you to infer the
>>> type being constructed, which makes the snippet there be equivalent to:
>>>
>>> ```
>>> metrics.allocated.put(
>>> name,
>>> Gauge(
>>> strings::join("/", "allocator/allocated", name),
>>> process::defer(self(), [this, name]() {
>>> double result = 0;
>>> foreachvalue (const Slave& slave, this->slaves) {
>>> Option<Value::Scalar> value =
>>> slave.allocated.get<Value::Scalar>(name);
>>> if (value.isSome()) {
>>> result += value.get().value();
>>> }
>>> }
>>> return result;
>>> })));
>>> ```
>>>
>>> What I’m proposing is to only allow this type of construction in a few
>>> cases, namely for tuples, pairs and initializer lists where it actually
>>> improves readability.
>>>
>>> Do you guys have any opinions on the matter?
>>
Re: On implicit construction of objects whose constructor takes
multiple parameters
Posted by Michael Park <mp...@apache.org>.
What I would propose is to update the style guide to say that a constructor
be marked *explicit* if implicit conversion or list-initialization is
undesired.
That is, rather than only marking single-argument constructors as
*explicit* which
only covers implicit conversions, we should also mark multi-argument
constructors as *explicit* if desired, to also cover list-initialization.
In this specific case, if you don't want to construct *Gauge* with {x, y,
z}, rather than everyone having to remember at every call-site to say
*Gauge*, they can be enforced that by marking *Gauge*'s constructor
*explicit.*
On 1 March 2016 at 11:24, Michael Browning <in...@gmail.com> wrote:
> I've seen braced initialization in a lot of contexts where the class of the
> object being initialized doesn't define an initializer_list constructor, so
> in that sense I think it's an idiomatic usage, and it has the advantage of
> disallowing narrowing implicit conversions like double to int. On the other
> hand, widespread use of braced initialization can cause some pain when you
> inadvertently use it for a type that does take an initializer_list when you
> meant to refer to a different constructor. I think the the latter case is
> worth avoiding, and in my opinion your proposal is in line with the spirit
> of the Mesos style guide as regards type deduction using auto, where it
> states:
>
> The main goal is to increase code readability. This is safely the case if
> the exact same type omitted on the left is already fully stated on the
> right.
>
>
> In your example case with the braced initializer, the type isn't stated. I
> agree that the explicit construction should be required.
>
> On Mon, Feb 29, 2016 at 3:14 PM, Alexander Rojas <al...@mesosphere.io>
> wrote:
>
> > Hi Guys,
> >
> > Today I was making a code review and I came across the following snippet:
> >
> > ```
> > metrics.allocated.put(
> > name,
> > {strings::join("/", "allocator/allocated", name),
> > process::defer(self(), [this, name]() {
> > double result = 0;
> > foreachvalue (const Slave& slave, this->slaves) {
> > Option<Value::Scalar> value =
> > slave.allocated.get<Value::Scalar>(name);
> > if (value.isSome()) {
> > result += value.get().value();
> > }
> > }
> > return result;
> > })});
> > ```
> >
> > I think by the code here is hard to tell what is happening here. If you
> > look at the declaration of `metrics.allocated` somewhere else you notice
> > that allocated has the following declaration:
> >
> > ```
> > hashmap<std::string, process::metrics::Gauge> total;
> > ```
> >
> > And gauge is certainly no container type, nor its constructor takes an
> > `std::initializer_list` as a parameter. In fact its signature is:
> >
> > ```
> > Gauge::Gauge(const std::string& name, const Deferred<Future<double>()>&
> f)
> > ```
> >
> > What is happening here is that brace constructors allows you to infer the
> > type being constructed, which makes the snippet there be equivalent to:
> >
> > ```
> > metrics.allocated.put(
> > name,
> > Gauge(
> > strings::join("/", "allocator/allocated", name),
> > process::defer(self(), [this, name]() {
> > double result = 0;
> > foreachvalue (const Slave& slave, this->slaves) {
> > Option<Value::Scalar> value =
> > slave.allocated.get<Value::Scalar>(name);
> > if (value.isSome()) {
> > result += value.get().value();
> > }
> > }
> > return result;
> > })));
> > ```
> >
> > What I’m proposing is to only allow this type of construction in a few
> > cases, namely for tuples, pairs and initializer lists where it actually
> > improves readability.
> >
> > Do you guys have any opinions on the matter?
>
Re: On implicit construction of objects whose constructor takes
multiple parameters
Posted by Michael Browning <in...@gmail.com>.
I've seen braced initialization in a lot of contexts where the class of the
object being initialized doesn't define an initializer_list constructor, so
in that sense I think it's an idiomatic usage, and it has the advantage of
disallowing narrowing implicit conversions like double to int. On the other
hand, widespread use of braced initialization can cause some pain when you
inadvertently use it for a type that does take an initializer_list when you
meant to refer to a different constructor. I think the the latter case is
worth avoiding, and in my opinion your proposal is in line with the spirit
of the Mesos style guide as regards type deduction using auto, where it
states:
The main goal is to increase code readability. This is safely the case if
the exact same type omitted on the left is already fully stated on the
right.
In your example case with the braced initializer, the type isn't stated. I
agree that the explicit construction should be required.
On Mon, Feb 29, 2016 at 3:14 PM, Alexander Rojas <al...@mesosphere.io>
wrote:
> Hi Guys,
>
> Today I was making a code review and I came across the following snippet:
>
> ```
> metrics.allocated.put(
> name,
> {strings::join("/", "allocator/allocated", name),
> process::defer(self(), [this, name]() {
> double result = 0;
> foreachvalue (const Slave& slave, this->slaves) {
> Option<Value::Scalar> value =
> slave.allocated.get<Value::Scalar>(name);
> if (value.isSome()) {
> result += value.get().value();
> }
> }
> return result;
> })});
> ```
>
> I think by the code here is hard to tell what is happening here. If you
> look at the declaration of `metrics.allocated` somewhere else you notice
> that allocated has the following declaration:
>
> ```
> hashmap<std::string, process::metrics::Gauge> total;
> ```
>
> And gauge is certainly no container type, nor its constructor takes an
> `std::initializer_list` as a parameter. In fact its signature is:
>
> ```
> Gauge::Gauge(const std::string& name, const Deferred<Future<double>()>& f)
> ```
>
> What is happening here is that brace constructors allows you to infer the
> type being constructed, which makes the snippet there be equivalent to:
>
> ```
> metrics.allocated.put(
> name,
> Gauge(
> strings::join("/", "allocator/allocated", name),
> process::defer(self(), [this, name]() {
> double result = 0;
> foreachvalue (const Slave& slave, this->slaves) {
> Option<Value::Scalar> value =
> slave.allocated.get<Value::Scalar>(name);
> if (value.isSome()) {
> result += value.get().value();
> }
> }
> return result;
> })));
> ```
>
> What I’m proposing is to only allow this type of construction in a few
> cases, namely for tuples, pairs and initializer lists where it actually
> improves readability.
>
> Do you guys have any opinions on the matter?