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 <[email protected]> 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 <[email protected]>
> 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?
>
