Re: [lmi] Naming certain overloaded functions

[Greg Chicares]

On 2006-10-26 13:19 UTC, Vadim Zeitlin wrote:
> On Thu, 26 Oct 2006 13:00:22 +0000 Greg Chicares <address@hidden> wrote:

[inserted to clarify discussion below, and slightly simplified:]

class streamable {
  public:
    virtual void write(xml_lmi::Element&) const = 0;
// No other 'write' function in this base class.
};

class Ledger: virtual public streamable {...}

> GC> Class Ledger has [realigned for emphasis]:
> GC>     virtual void   write(xml_lmi::Element&) const;
> GC>     void           write(std::ostream& os) const;
> GC>     void        do_write(xml_lmi::Element&, bool light_version) const;
> GC>                 ???^^^^^
> GC> Is there any objection to changing the last to
> GC>     void           write(xml_lmi::Element&, bool light_version) const;
> GC> so that they're all overloads of 'write'?
> 
>  It's in general a bad idea to have a virtual function in a set of
> overloads. This is because if [one of] the virtual methods is overloaded in
> a derived class, it hides all the other overloads (this is known as a
> "virtual function hiding problem" in C++) making it impossible to use them.

I thought the problem was this:

struct Base {
  virtual foo(int);
  virtual foo(double);
};

struct Derived: Base {
  virtual foo(int); // Now Base::foo(double) is hidden.
};

I had taken it as a theorem that this problem can be avoided by
implementing all virtuals in the derived class; and as a corollary
that it can be avoided by making all virtuals pure in the base class.
Is that wrong?

> There are, of course, workarounds:

Which, as you point out, are better avoided.

>  Which is why I strongly prefer to make write() functions non-virtual and
> have a single virtual do_write() usually. But, of course, here this is
> impossible as we inherit the virtual write() from the base class.

We can change that base class. I see you post an idea below, so
let me sketch mine here before I examine yours:

class streamable {
  public:
    void write(xml_lmi::Element&) const; {/* calls do_write() */}
  private:
    virtual void do_write(xml_lmi::Element&) const = 0;
};

class Ledger: public streamable {
  private:
    virtual void do_write(xml_lmi::Element&) const {/* implement here */}
};

I've done that in other places, but it seemed like overkill here--I
don't find the purpose given in this paragraph quoted from Kanze:

http://groups.google.com/group/comp.lang.c++.moderated/msg/ccfdf0e8beb6f5bb
|
| >>my usual use is not for customizing.  The base class is conceptually
| >>an interface, with *NO* behavior of its own.  The non-virtual public
| >>functions are there to enforce the contract specified by the
| >>interface, or to instrument the interface, and for no other reason.

In this thread, I think Henney considered Sutter to be advocating
too-broad use of NVI, and I've tried not to use it promiscuously
since I read that.

>  Summarizing all this, I'd propose:
> 
>       virtual void write(xml_lmi::Element&) const;
>       void output(std::ostream& os) const;
> 
>       // in private part
>       void do_write(xml_lmi::Element&, ...) const;

I believe it's more usual to use a 'do_' prefix on the pure virtual.
When I see Derived::do_foo(), I expect
  Base::foo();    // public and nonvirtual
  Base::do_foo(); // virtual and nonpublic
That's what 'do_'- means to me, and I think I'm not alone. If it had
been 'perform_write' or 'write_selectively', I wouldn't have noticed.

But wait--are we talking about the same thing at all? Rereading
this, I think your

> impossible as we inherit the virtual write() from the base class.

means not that you wish Base::write() were nonvirtual, but
rather that Base::write() is not overloaded, and shouldn't be.

Am I understanding this correctly now?

> I think keeping do_write() name makes sense here because, although write()
> is virtual and it is not, it's still the real implementation of write()
> which is just a wrapper here. But if there is another convention for
> calling such kind of functions in LMI already (e.g. write_impl() or
> write_internal() or whatever) then it should be used, of course.

I don't have any convention in mind. Anything but 'do_'- or 'Do'-.
Or, I guess, -'impl' because that seems to imply envelope-letter
(I can't bring myself to use Sutter's term that means 'acne lesion').
As for just plain 'write' with no prefix...

[reordered]
> But then
> in this case it just doesn't seem like a good idea to have the std::ostream
> overload anyhow: the 2 functions do quite different things AFAICS so it's
> confusing that they have the same name.

    virtual void write(xml_lmi::Element&) const;
            void write(std::ostream&    ) const;

They both write *this to somewhere. One writes to an xml element;
the other, to a stream. Why is that a bad use of overloading?

>  Finally, completely unrelated to the overloaded functions names, but still
> related to this do_write() function, I'd like to also change its
> parameters. This is my pet peeve so please bear with me if you heard it
> before (Evgeniy definitely did) but using bool parameters for things other
> than obvious on/off switches is simply evil, please see my attempt to
> explain why here:
> 
>       http://www.wxwidgets.org/develop/standard.htm#no_bool_params

Thanks: I don't think I'd seen that document before.

I completely agree. Recently, I began to change the interface of
class calendar_date--but in another trunk, so lmi's HEAD and
branches still have this:

  calendar_date add_years
    (calendar_date const& date
    ,int                  n_years
    ,bool                 is_curtate
    );

  int attained_age
    (calendar_date const& birthdate
    ,calendar_date const& as_of_date
    ,bool                 use_age_nearest_birthday
    );

So if I want to know how old you'll be N years hence, it's
  attained_age(birthdate, add_years(today, N, XXX), YYY);
where bool parameters XXX and YYY are some combination of
'true' and 'false'. I made mistakes: which of XXX and YYY
should I change to get the age as of the nearest birthday?
And does 'false' mean 'curtate' or not? That's the problem
you explain as "Readability".

As for your "Extensibility" issue: here, it turns out that
there are half a dozen meaningful values for parameter
    bool use_age_nearest_birthday
even though we've only needed two...so far...except that I
noticed a piece of code that smelled bad, and making it
produce a testable error required implementing a third.

Another advantage of enumerators is that they're greppable.

> So I strongly propose that do_write() signature be changed to
> 
>       enum xmlStyle
>       {
>               xmlStyle_Light,
>               xmlStyle_Full
>       };
> 
>       void do_write(xml_lmi::Element&, xmlStyle style) const;

I embrace the principle, but would ask you to write it this way:

enum enum_xml_style
    {e_xml_light
    ,e_xml_heavy
    };

to follow a convention that, though undocumented [I'm editing
things like this into 'style.html' as they come up] and perhaps
not ideal, is extremely widespread in 'lmi'. It's
  enum enum_* {e_*, e_*};
that's important; longer, more-descriptive enumerator-names like
  e_minimal_dataset
are often a good idea, and can make it unnecessary to repeat a
lexeme redundantly: i.e., I wouldn't insist on
  enum enum_xml_* {e_xml_*, e_xml_*};
                     ^^^^     ^^^^
at all.