[squid-dev] [PATCH] YesNoNone class updates

[Alex Rousskov]

On 01/11/2016 06:36 PM, Amos Jeffries wrote:
> Update class YesNoNone to use C++11 features which allow better boolean
> conversion, assignment, copy, and move ctors.

The safer boolean operator is welcomed, of course.


> +    YesNoNone &operator =(bool beSet) {configure(beSet); return *this;}

This is a bad idea IMO. If we want to distinguish configuration-set
options from code-set options, we should not have implicit conversions
like that. The caller should use the existing configure() method instead.



> +    explicit YesNoNone(const YesNoNone &) = default;
> +    YesNoNone &operator =(const YesNoNone &) = default;
> +
> +    explicit YesNoNone(YesNoNone &&) = default;
> +    YesNoNone &operator =(YesNoNone &&) = default;
> +
> +    ~YesNoNone() = default;

All the added "= default" methods should be removed as well AFAICT. We
are not violating Big Three or Big Five rules without them, and adding
them misleads the reader into thinking there is something complex about
this class internal data structures and memory usage.


> Distinction is made between implicit and explicit configuration.

Why do we need this increased complexity? Your patch does not seem to
show any places where this will be used. I am not saying this added
complexity is necessarily wrong, but it is missing the motivation/usage
examples to justify it.

If we do need to distinguish configuration-set from code-set options, a
different implementation is needed IMO: Instead of using three awkward
values for "beSet" and a hard-to-guess-when-to-check boolean "implicit"
member, we should be using a simple boolean "isSet" and a tri-valued
"setHow" member with optUnset, optConfigured, optImplicit values (or
similar).



> +    /// \return true iff enabled; asserts if the option has not been configured
> +    explicit operator bool() const;
> +
> +    /// \return true iff disabled; asserts if the option has not been configured
> +    bool operator !() const;


The above descriptions became very misleading after your changes. The
[old] code is still correct -- it asserts on unspecified option values.
However, specifying the value no longer requires calling configure()
because you have added "implicit" configuration via constructor.
s/configured/specified/


> +    explicit YesNoNone(bool beSet);

Please add a description to say that this constructor specifies the
option value implicitly.


> +bool
> +YesNoNone::operator !() const
> +{
> +    assert(option != YesNoNone::optUnspecified);
> +    return option == YesNoNone::optDisabled;
>  }

Please never reverse the existing operator bool code. Inline the
negation of that operator call instead.


> +YesNoNone::YesNoNone(bool beSet):
> +    implicit(true)
> +{
> +    // do not use configure() so implicit remains unchanged.
> +    option = beSet ? YesNoNone::optEnabled : YesNoNone::optDisabled;
> +}

Please initialize both members in the initialization list. This
constructor should be inlined IMO.


> +    option = beSet ? YesNoNone::optEnabled : YesNoNone::optDisabled;

> +    option = beSet ? YesNoNone::optEnabled : YesNoNone::optDisabled;

> +    assert(option != YesNoNone::optUnspecified);
> +    return option == YesNoNone::optEnabled;

> +    assert(option != YesNoNone::optUnspecified);
> +    return option == YesNoNone::optDisabled;

> +    YesNoNone(): option(YesNoNone::optUnspecified), implicit(false) {}

Here and elsewhere, please avoid the "YesNoNone::" scope prefix inside
the YesNoNone class itself.


HTH,

Alex.