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