Re: [lmi] Naming certain overloaded functions

[Vadim Zeitlin]

On Thu, 26 Oct 2006 13:00:22 +0000 Greg Chicares <address@hidden> wrote:

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'?

 Hello,

 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.
There are, of course, workarounds: using declaration can be used to bring
the hidden base class functions in scope or, for an even more brute-force
workaround, wrappers forwarding to the base class may be derived in the
derived class for all hidden functions. But both workarounds have the
problem of having to do it for all hidden overloads and so being brittle as
it's highly likely that when someone adds a new overload to the base class
the derived class[es] will not be updated.

 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. 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.

 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 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.


 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

In this case, both of the reasons given at the URL above apply perfectly:

1. one has no idea what does do_write(el, true) means when reading the code
2. it's quite possible that we decide to implement an intermediate
   (semi-light?) version later and we wouldn't be able to do it with bool

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;

 Thanks,
VZ