Re: [lmi] [lmi-commits] master 084f1b49 5/5: Make a different virtual pure

[Greg Chicares]

On 7/12/22 11:34, Vadim Zeitlin wrote:
> On Mon, 11 Jul 2022 22:53:09 -0400 (EDT) Greg Chicares 
> <gchicares@sbcglobal.net> wrote:
> 
> GC> branch: master
> GC> commit 084f1b493d38aea81cb6efa174751bbb82ebaef2
> GC> Author: Gregory W. Chicares <gchicares@sbcglobal.net>
> GC> Commit: Gregory W. Chicares <gchicares@sbcglobal.net>
> GC> 
> GC>     Make a different virtual pure
> 
>  How was the choice of the function to be made pure done here? I.e. why did
> you decide to make allowed_keywords() pure and not, say, default_keyword()?

I picked the only one that would actually compile with no further changes.

Class payment_sequence overrides only these three member functions:
    numeric_values_are_allowable() const override
    keyword_values_are_allowable() const override
    allowed_keywords() const override
and the first two are called by assert_sanity(), so they mustn't be pure.

> Or, maybe better, both? I do see that default_keyword() is not overridden
> in numeric_sequence, unlike allowed_keywords() which is, but this doesn't
> seem a good reason for determining whether the function should be pure
> virtual or not.

default_keyword() is used only with specific derived classes: e.g.,
when displaying mode or dbopt in the GUI, only keywords are allowed;
blank, being invalid, is never allowed; therefore those classes need
a default keyword, whereas other's don't.

allowed_keywords() is used much more broadly.

> The usual question to ask is whether the default behaviour
> makes sense or if the function absolutely must be overridden because there
> is no reasonable default implementation and from this point of view it
> seems quite strange to have allowed_keywords() to be pure virtual but
> default_keyword() not to be.

I'd frame that in probabilistic rather than absolute terms. The base
class's default_keyword() implementation is appropriate in most cases.
Its allowed_keyword() implementation is almost always customized:
only class numeric_sequence uses the base class's implementation.
Therefore, there's a stronger case for making the latter pure.

>  FWIW I'd probably make _all_ virtual functions here pure

Try it:
  pure virtual method called
  terminate called without an active exception

> because it
> doesn't seem like there is any natural default implementation for any of
> them.

    virtual std::string const default_keyword() const;
    virtual std::map<std::string,std::string> const allowed_keywords() const;

Natural default is empty.

    virtual bool keyword_values_are_allowable() const;

Natural default is false, corresponding to "empty" above.

    virtual bool numeric_values_are_allowable() const;

Natural default is true in light of the above considerations,
because one of the last two must be true (see assert_sanity()).

> GC>     When at least one virtual must be declared pure because a base class
> GC>     shouldn't be instantiated directly, it is conventional to put the
> GC>     pure-specifier on the dtor. This commit challenges that convention.
> 
>  I'd like to challenge this convention from the other direction: if the
> goal is to ensure that the class can't be instantiated directly, why not
> make (all of) its ctor(s) protected rather than public? This achieves the
> same effect in a much more clear way.

Committed; to be pushed after next test cycle, which begins now
because I need to be AFK for half an hour anyway.

That's much better. I simply hadn't thought of it.

> GC>     C++20 says [class.abstract/2]: "A function declaration cannot provide
> GC>     both a pure-specifier and a definition." Formerly, the dtor was
> GC>     declared as pure, and elsewhere defined as defaulted, with comments
> GC>     in both places. Code that requires comments is worse than code that
> GC>     does not. Moving the pure-specifier made comments unnecessary. This
> GC>     is therefore a pure improvement.
> 
>  Sorry for ruining the word play with my overriding concern for clarity,
> but I'm really not sure that the new version is better. It is shorter, but
> getting rid of the comments comes at a price of leaving the reader puzzled
> about what's going on here.

Nothing ruined: specifying access with access-specifiers instead of
a haphazard member-declarator is a pure improvement.

> GC>     Forwent the usual space before "0" because it would have made the line
> GC>     81 characters wide. To avoid writing a line wider than 80 characters,
> GC>     in a file where that limit is otherwise respected except for the
> GC>     copyright dates, is more important than writing that customary space.
> 
>  This is less important, but I think it's better to have tolerance for 81
> character lines rather than use inconsistent spacing. It leaves an itch to
> correct it and, worse, means that this pure virtual function won't be found
> if you search for "= 0" as I sometimes do.

Oh. I hadn't thought of that problem. But that'll be reverted,
without exceeding the width of a Hollerith card.