Re: [Opencxx-users] Makefile.am fixes

[Grzegorz J. <ja...@he...>]

On Mon, 15 Sep 2003, Yann Dirson wrote:

> On Mon, Sep 15, 2003 at 08:53:28AM +0800, Grzegorz Jakacki wrote:
> > Why not something like this:
> >
> > class MyWalker : public Walker
> > {
> > public:
> >      Ptree* TranslateCast(Ptree* exp)
> >      {
> >          Ptree exp2 = BeforeTranslateCast(exp);
> >          Ptree* e = exp2->Nth(3);
> >          Ptree* e2 = AfterTranslateCastedExpression(Translate(BeforeTranslateCastedExpression(e)));
> >          return AfterTranslateCast(new PtreeCastExpr(TranslateCastType(exp2->First()),
> >      	       	   		                Ptree::ShallowSubst(e2, e, exp2->Cdr())));
> >      }
> >
> >      ... BeforeTranslateCast(...) { ... }
> >      ... AfterTranslateCastedExpression(...) { ... }
> >      ... AfterTranslateCast(...) { ... }
> > };
> >
> > AFAIU you get the same result without adding a hook.
>
> Yes, but that makes a modified copy of the original code, and will require
> updating to take advantage of any changes in the original...

You are right. I would say that this is acceptable on this scale, but that's
maybe because I am getting old. Let's try again.

I understand that you would like to add BeforeTransalteCast(),
AfterTranslateCastedExpression(), BeforeTransalteCastedExpression(),
TranslateCastType() and TranslateCastType() to the public interface of
Walker. I would like to be careful about it, as changes to interfaces are
difficult to reverse.

First reservation is: why do we need FIVE new interface functions? It
seems to me that at most two should be enough. Too many functions makes
interface diffucult to comprehend.

Second: function like 'BeforeTranslateCast()' seems to be a bad candidate for public
iface function, as it has very complicated postcondition. If you do not
return some very specific Ptree, the remaining code will fail. This has to
be documented and understood by the client, so I would like to avoid it if
possible.

From what I can see the problem originates from the fact, that
'TranslateCast()' violates Single Responsibility Principle. It not only
calls 'Translate()' on the expression under cast, but also takes care
of copying the resulting expression if necessary. Moreover, you would like
to extend its functionality to allow cast type customization.

So maybe we can go on with something like this:

    Ptree* OpencxxWalker::TranslateCast(Ptree* exp)
    {
        Ptree* castedExp   = exp->Nth(3);
        Ptree* castedType  = exp-> ??? ;
        Ptree* castedExp1  = TranslateCastedExpression(castedExp);
        Ptree* castedType  = TranslateCastedType(castedType);

        if (castedExp == castedExp1) ...
        /* the code that creates a copy with appropariate nodes */
        /* substituted if necessary */
        /* BTW: similar code appears in many plaeces; can we factor it out? */
    }

    Ptree* OpencxxWalker::TranslateCastedExpression(Ptree* e) //new iface function
    {
        return Translate(e);
    }

    Ptree* OpencxxWalker::TranslateCastedType(Ptree* e)  //new iface function
    {
        return e;
    }

In your walker:

    class MyWalker : virtual public Walker, private OpencxxWalker
    {
    public:

        Ptree* MyWalker::TranslateCast(Ptree* e)
        {
            return AfterTranslateCast(
                       OpencxxWalker::TranslateCast(
                           BeforeTranslateCast(e)));
        }

        Ptree* MyWalker::TranslateCastedExpression(Ptree* e)
        {
            return BeforeTranslateCastedExpression(
                       OpencxxWalker::TranslateCastedExpression(
                           AfterTranslateCastedExpression(e)));
        }

        Ptree* MyWalker::TranslateCastedType(Ptree* e)
        {
            ... // things you want to do with TranslateCastType().
            ... // Rely on OpencxxWalker::TranslateCastedType() if you
            ... // want to take advantage of future changes
            ... // in Walker.
        }

    private:
        ... BeforeTranslateCast(...) { ... }
        ... AfterTranslateCastedExpression(...) { ... }
        ... AfterTranslateCast(...) { ... }

    };

Perhaps we also can give more meaningful types to arguments of
TranslateCastedExpression() and TranslateCastedType().

What do you think?

>
>
> > Looks like we came to similar conclusions.
> >
> > The code that I wrote above works, but it violates Liskov Substitution
> > Principle --- the user cannot be sure about the behaviour of member
> > function 'TranslateCast()' when it is called via Walker*.
>
> Hm - what assumptions of Walker would this code break ?

See

  * Sutter "Exceptional C++" chapter 24

  * http://www.gotw.ca/gotw/014.htm

  * http://www.objectmentor.com/resources/articles/lsp.pdf

Regards
Grzegorz

##################################################################
# Grzegorz Jakacki                       Huada Electronic Design #
# Senior Engineer, CAD Dept.              1 Gaojiayuan, Chaoyang #
# tel. +86-10-64365577 x2074               Beijing 100015, China #
# Copyright (C) 2002 Grzegorz Jakacki, HED. All Rights Reserved. #
##################################################################