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

[Greg Chicares]

On 7/16/22 14:55, Vadim Zeitlin wrote:
> On Sat, 16 Jul 2022 00:00:09 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> GC> On 7/12/22 13:42, Greg Chicares wrote:
> GC> > On 7/12/22 11:34, Vadim Zeitlin wrote:
> GC> >> On Mon, 11 Jul 2022 22:53:09 -0400 (EDT) Greg Chicares 
> <gchicares@sbcglobal.net> wrote:
[...]
> GC> >> GC>     Make a different virtual pure
> GC> >> 
> GC> >>  How was the choice of the function to be made pure done here? I.e. 
> why did
> GC> >> you decide to make allowed_keywords() pure and not, say, 
> default_keyword()?
> GC> > 
> GC> > I picked the only one that would actually compile with no further 
> changes.
> GC> > 
> GC> > Class payment_sequence overrides only these three member functions:
> GC> >     numeric_values_are_allowable() const override
> GC> >     keyword_values_are_allowable() const override
> GC> >     allowed_keywords() const override
> GC> > and the first two are called by assert_sanity(), so they mustn't be 
> pure.
> GC> 
> GC> Consider assert_sanity(). Aside from block_keyword_values()
> GC> (which was used in the past, but is no longer used), only
> GC> base-class ctors call assert_sanity(). Because it's called
> GC> only in base-class ctors, it tests the sanity only of the
> GC> base-class state--the derived classes override the function
> GC> calls it makes, but no derived class exists at this point.
> GC> 
> GC> This defect should be easy to fix.

Pushed just now:
  9effa535d..a984aa65d  master -> master

>  I'm not sure how exactly do you plan to fix this, but ideal would be,
> IMNSHO, to get rid of assert_sanity() completely and make sure that the
> impossible ("insane") case simply cannot arise.

It does not arise in the present code, which is highly unlikely
ever to be extended.

And the conditions for "sanity" have changed.

> Unfortunately I don't
> understand the purpose of this class fully enough to propose a concrete
> solution, e.g. I have no idea why do we have block_keyword_values()

Expunged.

>  But if I were writing a class like this today I wouldn't have two
> different interdependent virtual xxx_values_are_allowable() functions but
> would rather have just a single get_allowable_values_kind() returning an
> element of (scoped) enum with values Numeric, Keyword and Both (but not
> "None"). This would make it impossible to write, or at least to compile,
> insane code in the first place.

Interesting point. But if we were writing a set of classes like
this today, would we agree that inheritance is the best way to
tie them together?

>  Please let me know if you'd like me to try to make a patch or a branch
> with a series of patches implementing this approach.

No, thanks. It's not a bad idea, but the actual problem is fixed.