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