mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rojas <alexan...@mesosphere.io>
Subject Re: On implicit construction of objects whose constructor takes multiple parameters
Date Wed, 02 Mar 2016 18:27:57 GMT
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 <mpark@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 <invitapriore@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?
>> 


Mime
View raw message