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

[Vadim Zeitlin]

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()?
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. 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.

 FWIW I'd probably make _all_ virtual functions here pure because it
doesn't seem like there is any natural default implementation for any of
them.

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.

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.

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. Of course, I could search for
"= *0" instead, but this is not something I do naturally because there are
so few occurrences of "=0"s (I wanted to say "none", but decided to check
and there are actually 6 of them in wx own headers -- compared to 3159
occurrences of "= 0" in this context) in the code bases I'm working on.

 Regards,
VZ

pgp0GdCwnTSRH.pgp
Description: PGP signature