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

[Vadim Zeitlin]

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> >> GC> branch: master
GC> >> GC> commit 084f1b493d38aea81cb6efa174751bbb82ebaef2
GC> >> GC> Author: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> >> GC> Commit: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> >> GC> 
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.

 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. 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() when it
doesn't seem to be used anywhere since 68242d34d (Fix defect introduced
20050114T1947Z, 2010-07-04). And, of course, as always, I'm not sure about
the benefit of changing something that demonstrably works.

 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.

 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.

 Thanks,
VZ

pgpaG1qGQtf3m.pgp
Description: PGP signature