Re: Commit (HEAD): UT_Map destructor virtual

From: Joaquín Cuenca Abela (cuenca@pacaterie.u-psud.fr)
Date: Sat May 11 2002 - 17:28:39 EDT

  • Next message: Patrick Lam: "commit: cocoa timer implementation"

    On Sat, 2002-05-11 at 16:55, Christian Biesinger wrote:
    > On Sat, May 11, 2002 at 01:27:21PM +0200, Joaquín Cuenca Abela wrote:
    > > On Fri, 2002-05-10 at 21:41, Christian Biesinger wrote:
    > > > Make destructor of UT_Map virtual so that subclasses can override it and e.g. free the elements
    > > > CVS: ----------------------------------------------------------------------
    > > > CVS: Enter Log. Lines beginning with `CVS:' are removed automatically
    > > > CVS:
    > > > CVS: Committing in .
    > > > CVS:
    > > > CVS: Modified Files:
    > > > CVS: ut_map.h
    > > > CVS: ----------------------------------------------------------------------
    > >
    > > Please, remove this patch.
    > >
    > > UT_Map destructor should not be virtual (it was not virtual in the
    > > beginning on purpose). Consider it a "final" class.
    >
    > Why that? Being able to override the constructor would make some things so
    > much easier.

    sadly, sometimes the easy way is not the right one. This one is just
    one example.

    > Is the overhead of the vtable so bad?

    No. It's not a overhead problem (but for some very little classes it
    may be one).

    The problem is that if you use inheritance to do that, you will be
    breaking the Liskov Substitution Principle.

    Suppose that we have a class somewhere like this one:

    ==========================================================

    class A
    {
    public:
            A() : m_st(new char[20]) {}
            ~A() { delete[] m_st; }

            const char* getSt() const { return m_st; }

            void f(UT_Map* pm)
            {
                    pm->insert((UT_Map::key_t) getSt(), (UT_Map::data_t) 3);
            }

    private:
            char* m_st;
    };

    ============================================================

    You may see that this code was working perfectly (modulo compilation
    bugs, I've not tested it) with the old UT_Map.

    If you inherit from UT_Map and create, say, UT_OwnMap (like UT_Map but
    that takes ownership of keys and values), and you pass and object of
    UT_OwnMap to f (instead of an UT_Map), then the code will not work
    anymore.

    Idem if you have client code that inserts integers as keys.

    In short, your class _IS_NOT_A_ UT_Map, because it's false that
    "everything that it's true for a UT_Map, it's also true for a
    UT_OwnMap". And thus you should not use public inheritance to model the
    relationship between UT_OwnMap and UT_Map.

    In a more general way, you're trying to use inheritance to reuse
    somebody else code, instead of using inheritance to be reused by
    somebody else.

    Anyway, you have a genuine problem that requires a solution, and the
    most simple the solution (as far as it's a right solution), the better.

    To free all the items in a UT_Map, you only need the public API. That
    shows us that we don't need to add another function to UT_Map.
    Something as:

    void delete_keys_map(UT_Map& m)
    {
            Iterator end(m.end());
            for (Iterator it(m.begin()); it != end; ++it)
                    delete (YOUR_TYPE*) ((UT_Pair*) it.value)->first);
    }

    (you may want to use C++ style casts. You will need 4 C++ casts (2
    statics, and 2 consts). That shows how ugly is to use void*
    pointers...)

    The problem with this approach is that you may forget to call
    delete_keys_map, so if I were doing the work, I would write an adaptor
    to encapsulate this delete. Something as:

    class UT_OwnMap
    {
    public:
            UT_OwnMap(UT_Map::comparator comp) : m(comp) {}
            ~UT_OwnMap() {
                    ... delete the keys of m ...
            }

            ...inline methods to UT_Map methods...

    private:
            UT_Map m;
    };

    or you may make an accessor to the inner UT_Map, or ...

    The choice is yours.

    If you need further advice, don't hesitate to ask. If you want an
    implementation of UT_OwnMap, just ask and I will commit one. If you
    want something different, just ask.

    > Would it be at least possible to add a possibility for UT_Map to
    > automatically free its elements when it's destructed?
    >
    > > In the future, please consult any changes to UT_Map with me before
    > > committing.
    >
    > Sorry for that; I consulted dom and thought that was sufficient.

    Try to consult the original author first. Dom and Martin are your best
    shot if you don't know who is the original author of the code, but in
    this case my name is clearly stated in the copyright notice.

    Usually it doesn't matters too much, so if you're not able to contact
    the original author (because he doesn't reply or something), just do
    whatever you think is the best.

    Keep up the good work,

    Cheers,

    -- 
    Joaquín Cuenca Abela
    cuenca@pacaterie.u-psud.fr
    


    This archive was generated by hypermail 2.1.4 : Sat May 11 2002 - 17:27:52 EDT