delete a composite

[Matan Nassau]

This question is a little embarrassing... I have a boolean expressions
interpreter with its composite (see Go4's "Design Patterns" book).
here is a simple leaf of the composite:

class Constant : public BooleanExp {
public:
Constant(bool);
virtual ~Constant();
virtual void Accept(BooleanExpVisitor&);

bool GetValue() const {return _value;}

private:
bool _value;
};

and here is a composite complying with the BooleanExp interface:

class AndExp : public BooleanExp {
public:
AndExp(BooleanExp*, BooleanExp*);
virtual ~AndExp();
virtual void Accept(BooleanExpVisitor&);

private:
BooleanExp* _operand1;
BooleanExp* _operand2;
};

Consider a few other classes that are similar in concept, including
OrExp, VariableExp and so on. The Context class maps variables to
their boolean values.
Now I want to evaluate an expression:

BooleanExp *expression;
Context cont;

context.Assign(x, false);
context.Assign(y, true);

EvaluationVisitor visitor(cont);

expression = new OrExp (
new AndExp (new Constant (true), x),
new AndExp (y, new NotExp (x))
);

bool result = expression->Accept(visitor);

Up to here everything is good. My question is, how do I collect the
garbage?
What I have at the moment is

delete expression;

Do you think this will do?
I have a virtual destructor for each of the BooleanExp (concrete)
subclasses, and they all do nothing. Correct me if I'm wrong, but I
don't think it's a good idea to let these destructors have the
responsibility to clean up since the dynamic allocations are performed
out of the class.

TIA,

Matan Nassau

 

[John Harrison]

This question is a little embarrassing... I have a boolean expressions
interpreter with its composite (see Go4's "Design Patterns" book).
here is a simple leaf of the composite:

class Constant : public BooleanExp {
public:
Constant(bool);
virtual ~Constant();
virtual void Accept(BooleanExpVisitor&);

bool GetValue() const {return _value;}

private:
bool _value;
};

and here is a composite complying with the BooleanExp interface:

class AndExp : public BooleanExp {
public:
AndExp(BooleanExp*, BooleanExp*);
virtual ~AndExp();
virtual void Accept(BooleanExpVisitor&);

private:
BooleanExp* _operand1;
BooleanExp* _operand2;
};

Consider a few other classes that are similar in concept, including
OrExp, VariableExp and so on. The Context class maps variables to
their boolean values.
Now I want to evaluate an expression:

BooleanExp *expression;
Context cont;

context.Assign(x, false);
context.Assign(y, true);

EvaluationVisitor visitor(cont);

expression = new OrExp (
new AndExp (new Constant (true), x),
new AndExp (y, new NotExp (x))
);

bool result = expression->Accept(visitor);

Up to here everything is good. My question is, how do I collect the
garbage?
What I have at the moment is

delete expression;

Do you think this will do?

It won't free the allocated memory.

I have a virtual destructor for each of the BooleanExp (concrete)
subclasses, and they all do nothing. Correct me if I'm wrong, but I
don't think it's a good idea to let these destructors have the
responsibility to clean up since the dynamic allocations are performed
out of the class.

It's quite common for classes to 'take ownership' of pointers in this way.
It's the basis of most smart pointer classes for instance. I would do the
deletes in the destructors, anything else is going to be very tedious and
therefore very error prone.

john

 

[Victor Bazarov]

This question is a little embarrassing... I have a boolean expressions
interpreter with its composite (see Go4's "Design Patterns" book).
here is a simple leaf of the composite:

class Constant : public BooleanExp {
public:
Constant(bool);
virtual ~Constant();
virtual void Accept(BooleanExpVisitor&);

bool GetValue() const {return _value;}

private:
bool _value;
};

and here is a composite complying with the BooleanExp interface:

class AndExp : public BooleanExp {
public:
AndExp(BooleanExp*, BooleanExp*);
virtual ~AndExp();
virtual void Accept(BooleanExpVisitor&);

private:
BooleanExp* _operand1;
BooleanExp* _operand2;
};

Consider a few other classes that are similar in concept, including
OrExp, VariableExp and so on. The Context class maps variables to
their boolean values.
Now I want to evaluate an expression:

BooleanExp *expression;
Context cont;

context.Assign(x, false);
context.Assign(y, true);

EvaluationVisitor visitor(cont);

expression = new OrExp (
new AndExp (new Constant (true), x),
new AndExp (y, new NotExp (x))
);

bool result = expression->Accept(visitor);

Up to here everything is good. My question is, how do I collect the
garbage?

It depends on who [you think] owns those pointers. The owner should
dispose of them. There is no "garbage collection", everybody disposes of
their own rubbish.

What I have at the moment is

delete expression;

Do you think this will do?

How should we know? You posted no implementation of your classes. Does
'AndExp' own the object, to which pointers 'operand_1' and 'operand_2'
point? If so, it should dispose of them in the destructor.

I have a virtual destructor for each of the BooleanExp (concrete)
subclasses, and they all do nothing. Correct me if I'm wrong, but I
don't think it's a good idea to let these destructors have the
responsibility to clean up since the dynamic allocations are performed
out of the class.

Dynamic allocations for std::auto_ptr are performed "out of the class" but
it doesn't prevent std::auto_ptr from being responsible of disposing the
object. It all depends on what responsibilities you assign to your classes.

Don't forget to document them.

Victor

 

[WT]

Thank you very much, Victor, and you, John, for your fast replies.

How should we know? You posted no implementation of your classes. Does
'AndExp' own the object, to which pointers 'operand_1' and 'operand_2'
point? If so, it should dispose of them in the destructor.

I posted the implementations that I thought were important for the question,
and I ommited others.
OrExp is realy similar to AndExp. I could actually say that the two are
identical, and the only difference in behavior is actually in the visitor,
which ORs the two operands instead of ANDs them when OrExp accepts the
visit.
Here is the Accept operation of OrExp:

void OrExp::Accept(BooleanExpVisitor &visitor)
{
_operand1->Accept(visitor);
_operand2->Accept(visitor);
visitor.VisitOrExp(this);
}

The visitor class should be provided as well, I apologize.

class EvaluationVisitor : public BooleanExpVisitor {
public:
EvaluationVisitor(Context&);
~EvaluationVisitor();
bool PopResult();
virtual void VisitVariableExp(VariableExp*);
virtual void VisitConstant(Constant*);
virtual void VisitAndExp(AndExp*);
virtual void VisitOrExp(OrExp*);
virtual void VisitNotExp(NotExp*);
private:
Context& _context;
deque <bool> _result;
};

The class complies with the BooleanExpVisitor interface. it overrides its
defaults. PopResult pops the last result out of the _result stack.
Here's VisitOrExp:

void EvaluationVisitor::VisitOrExp(OrExp *exp)
{
if (_result.empty())
throw false;
// pop, pop, evaluate and push
bool val2 = _result.back();
_result.pop_back();
bool val1 = _result.back();
_result.pop_back();
_result.push_back( val1 || val2 );
}

VisitVariableExp will find the value of the variable in the context map,
which is a member of the visitor, and will push it into the stack.
VisitConstant will get the constant value and push it into the stack.
NotExp is the same class as OrExp and AndExp with one operand instead of 2,
and another difference in the visitor which NOTs the operand of the NotExp
object.
Constant is a leaf similar to VariableExp, with the 2 differences similiar
to those above: a bool member variable which holds its value, instead of a
char* which holds the variable's name.
Context maps variable names to their boolean values, and it is useful to
change the result of an expression without changing the expression. It has
the following interface:

class Context {
public:
bool Lookup(const char*) const;
void Assign(VariableExp*, bool);
private:
map<const char*, bool> _theMap;
};

I think the interface speaks for itself. It's pretty much a wrapper for the
map, only providing just the necessary manipulation methods.

Dynamic allocations for std::auto_ptr are performed "out of the class" but
it doesn't prevent std::auto_ptr from being responsible of disposing the
object.

That's also what John said. But I think this is because that is exactly the
idea behind smart pointers: take over the responsibility of cleaning up, and
do it safely. What I was hesitating about, was what you mentioned next:

It all depends on what responsibilities you assign to your classes.

my classes don't have to help the client with anything other than
interpreting boolean expressions. if they make a mess, ofcourse they must
clean after. However, at the moment it's the client which allocates the
objects that construct the composite. I could go John's way, and delete
everything in the the constructors, at the price of safety (any other price
I'm unaware of?). Or I could go with the more "apathetic" attitude, allowing
the client to allocate objects - but not cleaning up after it. I think it
realy is a matter of opinion, and there is no strict answer to that...

Don't forget to document them.

Good practice is what I'm practicing. Documentation is a good practice
indeed.

Matan

 

[Matan Nassau]

Thank you very much, Victor, and you, John, for your fast replies.

How should we know? You posted no implementation of your classes. Does
'AndExp' own the object, to which pointers 'operand_1' and 'operand_2'
point? If so, it should dispose of them in the destructor.

I posted the implementations that I thought were important for the question,
and I ommited others.
OrExp is realy similar to AndExp. I could actually say that the two are
identical, and the only difference in behavior is actually in the visitor,
which ORs the two operands instead of ANDs them when OrExp accepts the
visit.
Here is the Accept operation of OrExp:

void OrExp::Accept(BooleanExpVisitor &visitor)
{
_operand1->Accept(visitor);
_operand2->Accept(visitor);
visitor.VisitOrExp(this);
}

The visitor class should be provided as well, I apologize.

class EvaluationVisitor : public BooleanExpVisitor {
public:
EvaluationVisitor(Context&);
~EvaluationVisitor();
bool PopResult();
virtual void VisitVariableExp(VariableExp*);
virtual void VisitConstant(Constant*);
virtual void VisitAndExp(AndExp*);
virtual void VisitOrExp(OrExp*);
virtual void VisitNotExp(NotExp*);
private:
Context& _context;
deque <bool> _result;
};

The class complies with the BooleanExpVisitor interface. it overrides its
defaults. PopResult pops the last result out of the _result stack.
Here's VisitOrExp:

void EvaluationVisitor::VisitOrExp(OrExp *exp)
{
if (_result.empty())
throw false;
// pop, pop, evaluate and push
bool val2 = _result.back();
_result.pop_back();
bool val1 = _result.back();
_result.pop_back();
_result.push_back( val1 || val2 );
}

VisitVariableExp will find the value of the variable in the context map,
which is a member of the visitor, and will push it into the stack.
VisitConstant will get the constant value and push it into the stack.
NotExp is the same class as OrExp and AndExp with one operand instead of 2,
and another difference in the visitor which NOTs the operand of the NotExp
object.
Constant is a leaf similar to VariableExp, with the 2 differences similiar
to those above: a bool member variable which holds its value, instead of a
char* which holds the variable's name.
Context maps variable names to their boolean values, and it is useful to
change the result of an expression without changing the expression. It has
the following interface:

class Context {
public:
bool Lookup(const char*) const;
void Assign(VariableExp*, bool);
private:
map<const char*, bool> _theMap;
};

I think the interface speaks for itself. It's pretty much a wrapper for the
map, only providing just the necessary manipulation methods.

Dynamic allocations for std::auto_ptr are performed "out of the class" but
it doesn't prevent std::auto_ptr from being responsible of disposing the
object.

That's also what John said. But I think this is because that is exactly the
idea behind smart pointers: take over the responsibility of cleaning up, and
do it safely. What I was hesitating about, was what you mentioned next:

It all depends on what responsibilities you assign to your classes.

my classes don't have to help the client with anything other than
interpreting boolean expressions. if they make a mess, ofcourse they must
clean after. However, at the moment it's the client which allocates the
objects that construct the composite. I could go John's way, and delete
everything in the the constructors, at the price of safety (any other price
I'm unaware of?). Or I could go with the more "apathetic" attitude, allowing
the client to allocate objects - but not cleaning up after it. I think it
realy is a matter of opinion, and there is no strict answer to that...

Don't forget to document them.

Good practice is what I'm practicing. Documentation is a good practice
indeed.

Matan