Re: [lmi] Delegating ctor uncalled for?

[Greg Chicares]

On 2017-02-07 14:37, Vadim Zeitlin wrote:
> On Tue, 7 Feb 2017 04:58:33 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> Please comment on this attempt. No comment is too trivial.
[...]
> GC>      ValueInterval dummy;
> GC> -    dummy.value_is_keyword = true;
> GC> +    dummy.value_is_keyword = T_is_string;
> 
>  Here I want to argue in favour of setting value_is_keyword in set_value()
> itself, please see below.
[...]
>       C.2: Use class if the class has an invariant; use struct if the
>       data members can vary independently
> 
> (see http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-struct)
[...]
> according to C.2, ValueInterval must be a class because it does have an
> invariant: "value_keyword is valid iff value_is_keyword is true". And based
> on this I strongly believe that set_value() must modify value_is_keyword in
> addition to changing the value.

Yes, it is an invariant.

In this case, I agree with "must modify value_is_keyword" if it's
a postcondition of this function. Yet it could instead be treated
as a precondition...see below.

> In fact, I'd argue for using a
> variant<double, std::string> here instead of these 3 fields, to make things
> even more type-safe, but considering our reluctance to start using Boost
> libraries and impossibility to start using C++17 just yet, this is a
> non-starter, of course.

That's a header-only boost library, so I took a look at what other
parts of boost it brings in, and...no thanks. We can discuss this
again when we have a C++17 toolchain.

> GC> +// OTOH, classes shouldn't have public nonconst data members.
> 
>  I disagree with this. If it doesn't make sense to abstract it and if there
> are no invariants involving this member, there is no good reason to not
> provide public access to it.

I somewhat disagree. Including both public and private data in the
same class is less simple than making them all public or all private.
However...

>  So, combining both points, I would make ValueInterval a class, but keep
> all of its members except for value_xxx public, while providing get_value()
> accessors, checking that the correct field is being accessed, for
> value_{number,keyword}. Of course, this is not directly related to these
> changes, so it could be done before or after them (or not at all).
[...]
>  To summarize, there are 2 important points I tried to make:
[...]
> 2. Don't allow manipulating value_is_keyword independently but set it when
>    calling set_value(). Later it would be nice to make ValueInterval a
>    class and stop providing direct access to value_{number,keyword} to
>    ensure that the invariant can't be broken.

In this chain of commits, we've merged two ctors that were almost
identical (and expunged a third that was not useful), so we've
already accomplished everything that was most needed. How we set
ValueInterval members is either a minor detail of this translation
unit, or a more significant detail spanning multiple TUs, but either
way the cost-to-benefit ratio of further refinement is increasing...
while other improvements to this class will deliver more benefit
with less work.

And I don't really want to touch other stuff like the GUI input-
sequence editor at this time--at least not the way it sets
ValueInterval--so this all comes down to polishing just the two
remaining lines in the new duplication-free template function.
Therefore, I'm reverting to the least beautiful idea we've
discussed, because it is simple and robust, and asserting
is_keyword consistency as a precondition. See commit 39d1ed2.