Octave_value class hierachy

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Octave_value class hierachy

Jens Ruecknagel-2
Hi,

Why is count a attribute of octave_value? Why is ref  *octave_value?
Shouldn't it be: count attribute of octave_base_value and
octave_value::ref* octave_base_value?
Wouldn't it be better to make octave_value the proxy class and
octave_base_value the referenced class? It is used this way anyway?

I'm just curios? Does anybody know the reason, I did not think of?

Thanks Jens


Reply | Threaded
Open this post in threaded view
|

Octave_value class hierachy

John W. Eaton-6
On 22-Apr-2004, Jens Ruecknagel <[hidden email]> wrote:

| Why is count a attribute of octave_value? Why is ref  *octave_value?
| Shouldn't it be: count attribute of octave_base_value and
| octave_value::ref* octave_base_value?
| Wouldn't it be better to make octave_value the proxy class and
| octave_base_value the referenced class? It is used this way anyway?
|
| I'm just curios? Does anybody know the reason, I did not think of?

I think I followed the structure of the Number class hierarchy in
Coplien's "Advanced C++; Programming Styles and Idioms" book from
1992.  I must admit that at the time, that code seemed quite complex
to me, so probably I copied a lot more than I really understood.

Looking at the code now, I think the rep object must be a pointer to
an octave_value object so that the virtual functions in the
octave_value class work properly.

If you see another way to do this that is better, then please submit a
complete working example that demonstrates your ideas.  It can be
greatly simplified compared to the octave_value classes (for example,
just two derived types and one or two operations on them).  You could
start with the code in the first attachment below, and fix it so that
it works correctly.  Currently, both calls to the functions in main
end up in the methods of base_value class when only the one for
type_two should.  When you are done, I think you will probably end up
with something much like what is in the second attachment, which is a
simplified version of the current octave_value hierarchy.  But maybe
there is a simpler and better way that I'm not seeing.  In any case,
going through the exercise yourself will probably help you to
understand why things are the way they are.

jwe


#include <iostream>

class base_value;

class value
{
public:

  value (int t = 0);

  value (const value& v);

  value& operator = (const value& v);

  virtual ~value (void);

  virtual value fun (void) const;

private:

  base_value *rep;
};

class base_value
{
  friend class value;

public:

  base_value (void) { count = 1; }

  value fun (void) const
  {
    std::cerr << "base_value::fun" << std::endl;
    return value ();
  }

private:

  int count;
};

class type_one : public base_value
{
public:

  type_one (void) : base_value () { }

  value fun (void) const
  {
    std::cerr << "type_one::fun" << std::endl;
    return value ();
  }
};

class type_two : public base_value
{
public:

  type_two (void) : base_value () { }

  // Let the base class handle this one...
  // value fun (void) const
};

value::value (int t)
{
  switch (t)
    {
    case 1:
      rep = new type_one ();
      break;

    case 2:
      rep = new type_two ();
      break;

    default:
      rep = new base_value ();
      break;
    }
}

value::value (const value& v)
{
  rep = v.rep;
  rep->count++;
}

value&
value::operator = (const value& v)
{
  if (this != &v)
    {
      if (--rep->count == 0)
        delete rep;

      rep = v.rep;
      rep->count++;
    }
  return *this;
}

value::~value (void)
{
  if (--rep->count == 0)
    delete rep;
}

value
value::fun (void) const
{
  return rep->fun ();
}

int
main (void)
{
  value one (1);
  value two (2);

  // Should call type_one::fun
  one.fun ();

  // Should call base_value::fun, because type_two does not
  // implement fun.
  two.fun ();

  return 0;
}

#include <iostream>

class base_value;

class
xvalue
{
public:

  xvalue (void) { }
};

class value
{
public:

  value (int t = 0);

  value (const value& v)
  {
    rep = v.rep;
    rep->count++;
  }

  value& operator = (const value& v)
  {
    if (this != &v)
      {
        if (--rep->count == 0)
          delete rep;

        rep = v.rep;
        rep->count++;
      }
    return *this;
  }

  virtual ~value (void)
  {
    if (rep && --rep->count == 0)
      {
        delete rep;
        rep = 0;
      }
  }

  virtual value fun (void) const { return rep->fun (); }

private:

  union
  {
    value *rep;
    int count;
  };

protected:

  value (const xvalue&) : rep (0) { }
};

class base_value : public value
{
public:

  base_value (void) : value (xvalue ()) { }

  base_value (const base_value&) : value (xvalue ()) { }

  ~base_value (void) { }

  value fun (void) const
  {
    std::cerr << "base_value::fun" << std::endl;
    return value ();
  }
};

class type_one : public base_value
{
public:

  type_one (void) : base_value () { }

  ~type_one (void) { }

  value fun (void) const
  {
    std::cerr << "type_one::fun" << std::endl;
    return value ();
  }
};

class type_two : public base_value
{
public:

  type_two (void) : base_value () { }

  ~type_two (void) { }

  // Let the base class handle this one...
  // value fun (void) const
};

value::value (int t)
{
  switch (t)
    {
      case 2:
        rep = new type_two ();
        rep->count = 1;
        break;

    default:
      rep = new type_one ();
      rep->count = 1;
      break;
    }
}

int
main (void)
{
  value one (1);
  value two (2);

  // Should call type_one::fun
  one.fun ();

  // Should call base_value::fun, because type_two does not
  // implement fun.
  two.fun ();

  return 0;
}
Reply | Threaded
Open this post in threaded view
|

Re: Octave_value class hierachy

Jens Ruecknagel-2
Thanks for you answer.

your foo.cc is exactly what i was thinking about except one line:

diff foo.orig.cc foo.cc
32c32
<   value fun (void) const
---
 >   virtual value fun (void) const

in foo you cannot do what you can do in bar:
value crash = type_one () ;  // you know I cannot write "value crash(
type_one())" because in c++ this is a forward declaration of a function
crash

and in octave_value you can write:

octave_value crash (octave_scalar(1));

Because the of the union {count, rep} the copy constructor cannot find
out whether it is a value or a base_value.

Or because of base_value : public value and base_value is NOT a value,
value is just a a pointer to a base_value.
Or maybe it is even a proxy [Gama, Helm, Johnson, Vlissidis, Design
Patterns] to a base_value. If one wants, they may both (value and
base_value) share the same interface: inherit both from the abstract
class value_interface.

But what the big deal: octave_value works: just don't call functions of
octave_base_value or its descendants. But it took me one week to find
this out.


John W. Eaton wrote:

>On 22-Apr-2004, Jens Ruecknagel <[hidden email]> wrote:
>
>| Why is count a attribute of octave_value? Why is ref  *octave_value?
>| Shouldn't it be: count attribute of octave_base_value and
>| octave_value::ref* octave_base_value?
>| Wouldn't it be better to make octave_value the proxy class and
>| octave_base_value the referenced class? It is used this way anyway?
>|
>| I'm just curios? Does anybody know the reason, I did not think of?
>
>I think I followed the structure of the Number class hierarchy in
>Coplien's "Advanced C++; Programming Styles and Idioms" book from
>1992.  I must admit that at the time, that code seemed quite complex
>to me, so probably I copied a lot more than I really understood.
>
>Looking at the code now, I think the rep object must be a pointer to
>an octave_value object so that the virtual functions in the
>octave_value class work properly.
>
>If you see another way to do this that is better, then please submit a
>complete working example that demonstrates your ideas.  It can be
>greatly simplified compared to the octave_value classes (for example,
>just two derived types and one or two operations on them).  You could
>start with the code in the first attachment below, and fix it so that
>it works correctly.  Currently, both calls to the functions in main
>end up in the methods of base_value class when only the one for
>type_two should.  When you are done, I think you will probably end up
>with something much like what is in the second attachment, which is a
>simplified version of the current octave_value hierarchy.  But maybe
>there is a simpler and better way that I'm not seeing.  In any case,
>going through the exercise yourself will probably help you to
>understand why things are the way they are.
>
>jwe
>
>  
>
>------------------------------------------------------------------------
>
>  
>



Reply | Threaded
Open this post in threaded view
|

Re: Octave_value class hierachy

Paul Thomas-10
Thanks for this correspondence, fellas.  John's simplified examples came
just in time, as I was trying to come to grips with the octave-value
hierarchy.  I wonder if the examples should be posted in the Wiki or
some other indexed locatation?

Paul T

Jens Ruecknagel wrote:

> Thanks for you answer.
>
> your foo.cc is exactly what i was thinking about except one line:
>
> diff foo.orig.cc foo.cc
> 32c32
> <   value fun (void) const
> ---
> >   virtual value fun (void) const
>
> in foo you cannot do what you can do in bar:
> value crash = type_one () ;  // you know I cannot write "value crash(
> type_one())" because in c++ this is a forward declaration of a
> function crash
>
> and in octave_value you can write:
>
> octave_value crash (octave_scalar(1));
>
> Because the of the union {count, rep} the copy constructor cannot find
> out whether it is a value or a base_value.
>
> Or because of base_value : public value and base_value is NOT a value,
> value is just a a pointer to a base_value.
> Or maybe it is even a proxy [Gama, Helm, Johnson, Vlissidis, Design
> Patterns] to a base_value. If one wants, they may both (value and
> base_value) share the same interface: inherit both from the abstract
> class value_interface.
>
> But what the big deal: octave_value works: just don't call functions
> of octave_base_value or its descendants. But it took me one week to
> find this out.
>
>
> John W. Eaton wrote:
>
>> On 22-Apr-2004, Jens Ruecknagel <[hidden email]>
>> wrote:
>>
>> | Why is count a attribute of octave_value? Why is ref  *octave_value?
>> | Shouldn't it be: count attribute of octave_base_value and |
>> octave_value::ref* octave_base_value?
>> | Wouldn't it be better to make octave_value the proxy class and |
>> octave_base_value the referenced class? It is used this way anyway?
>> | | I'm just curios? Does anybody know the reason, I did not think of?
>>
>> I think I followed the structure of the Number class hierarchy in
>> Coplien's "Advanced C++; Programming Styles and Idioms" book from
>> 1992.  I must admit that at the time, that code seemed quite complex
>> to me, so probably I copied a lot more than I really understood.
>>
>> Looking at the code now, I think the rep object must be a pointer to
>> an octave_value object so that the virtual functions in the
>> octave_value class work properly.
>>
>> If you see another way to do this that is better, then please submit a
>> complete working example that demonstrates your ideas.  It can be
>> greatly simplified compared to the octave_value classes (for example,
>> just two derived types and one or two operations on them).  You could
>> start with the code in the first attachment below, and fix it so that
>> it works correctly.  Currently, both calls to the functions in main
>> end up in the methods of base_value class when only the one for
>> type_two should.  When you are done, I think you will probably end up
>> with something much like what is in the second attachment, which is a
>> simplified version of the current octave_value hierarchy.  But maybe
>> there is a simpler and better way that I'm not seeing.  In any case,
>> going through the exercise yourself will probably help you to
>> understand why things are the way they are.
>>
>> jwe
>>
>>  
>>
>> ------------------------------------------------------------------------
>>
>>  
>>
>
>
>



Reply | Threaded
Open this post in threaded view
|

Re: Octave_value class hierachy

John W. Eaton-6
In reply to this post by Jens Ruecknagel-2
On 24-Apr-2004, Jens Ruecknagel wrote:

| Thanks for you answer.
|
| your foo.cc is exactly what i was thinking about except one line:
|
| diff foo.orig.cc foo.cc
| 32c32
| <   value fun (void) const
| ---
|  >   virtual value fun (void) const
|
| in foo you cannot do what you can do in bar:
| value crash = type_one () ;  // you know I cannot write "value crash(
| type_one())" because in c++ this is a forward declaration of a function
| crash
|
| and in octave_value you can write:
|
| octave_value crash (octave_scalar(1));
|
| Because the of the union {count, rep} the copy constructor cannot find
| out whether it is a value or a base_value.
|
| Or because of base_value : public value and base_value is NOT a value,
| value is just a a pointer to a base_value.
| Or maybe it is even a proxy [Gama, Helm, Johnson, Vlissidis, Design
| Patterns] to a base_value. If one wants, they may both (value and
| base_value) share the same interface: inherit both from the abstract
| class value_interface.
|
| But what the big deal: octave_value works: just don't call functions of
| octave_base_value or its descendants. But it took me one week to find
| this out.

I'd like to revisit this topic now.  For extra context, see

  http://www.octave.org/mailing-lists/octave-maintainers/2004/437
  http://www.octave.org/mailing-lists/octave-maintainers/2004/443

I think your suggestion is good, and could simplify the implementation
of the octave_value classes a bit.

We would need to make the following changes:

  * octave_value::rep becomes a pointer to octave_base_value instead of
    octave_value

  * The octave_base_value class is no longer derived from octave_value.

  * All other value classes must be derived from octave_base_value (I
    think this is already true, but it would become a requirement).

  * Functions in the octave_value class would not need to be declared
    virtual, but the functions in octave_base_value would be virtual.

  * In addition to changing the type of octave_value::rep, we would
    also have

      typedef octave_base_value * (*type_conv_fcn) (const octave_value&);
      octave_value (octave_base_value *new_rep, int count = 1);
      octave_base_value *clone (void) const;
      octave_base_value *empty_clone (void) const;
      octave_base_value *try_narrowing_conversion (void);
      octave_base_value *internal_rep (void) const;

    in the octave_value class instead of

      typedef octave_value * (*type_conv_fcn) (const octave_value&);
      octave_value (octave_value *new_rep, int count = 1);
      virtual octave_value *clone (void) const;
      virtual octave_value *empty_clone (void) const;
      virtual octave_value *try_narrowing_conversion (void);
      octave_value *internal_rep (void) const;

    and the nil_rep function could be moved to the octave_base_value
    class.  These changes would affect all derived types, but should
    only require a change in the declaration, with no change in the
    way the functions actually work.

  * There would be no need for the octave_xvalue struct (which is just
    confusing anyway).

  * The rep could be assumed to always be non-null.

  * As Jens suggested, we could move count to the octave_base_value
    class.

Comments?

jwe

Reply | Threaded
Open this post in threaded view
|

Re: Octave_value class hierachy

John W. Eaton-6
On 16-Nov-2005, John W. Eaton wrote:

| We would need to make the following changes:
|
|   * octave_value::rep becomes a pointer to octave_base_value instead of
|     octave_value
|
|   * The octave_base_value class is no longer derived from octave_value.
|
|   * All other value classes must be derived from octave_base_value (I
|     think this is already true, but it would become a requirement).
|
|   * Functions in the octave_value class would not need to be declared
|     virtual, but the functions in octave_base_value would be virtual.
|
|   * In addition to changing the type of octave_value::rep, we would
|     also have
|
|       typedef octave_base_value * (*type_conv_fcn) (const octave_value&);
|       octave_value (octave_base_value *new_rep, int count = 1);
|       octave_base_value *clone (void) const;
|       octave_base_value *empty_clone (void) const;
|       octave_base_value *try_narrowing_conversion (void);
|       octave_base_value *internal_rep (void) const;
|
|     in the octave_value class instead of
|
|       typedef octave_value * (*type_conv_fcn) (const octave_value&);
|       octave_value (octave_value *new_rep, int count = 1);
|       virtual octave_value *clone (void) const;
|       virtual octave_value *empty_clone (void) const;
|       virtual octave_value *try_narrowing_conversion (void);
|       octave_value *internal_rep (void) const;
|
|     and the nil_rep function could be moved to the octave_base_value
|     class.  These changes would affect all derived types, but should
|     only require a change in the declaration, with no change in the
|     way the functions actually work.
|
|   * There would be no need for the octave_xvalue struct (which is just
|     confusing anyway).
|
|   * The rep could be assumed to always be non-null.
|
|   * As Jens suggested, we could move count to the octave_base_value
|     class.

OK, I spent some time making this change (not checked in yet; I may
put it on a separate branch).  It turned up some bugs (for example,
inappropriately casting an octave_value object to a derived type) and
some questionable design (the numeric_conversion_function method, for
one).

Changes needed in derived types are

  * The empty_clone, clone, try_narrowing_conversion functions return
    a pointer to octave_base_value instead of a pointer to octave_value.
    The body of the function will probably not need to change.

  * The type_conv_fcn typedef is now declared inside octave_base_value class,
    so numeric_conversion_function declaration must change.  This may
    change

  * The type_conv_fcn typedef changes from

      octave_value* (*) (const octave_value& a)

    to

      octave_base_value* (*) (const octave_base_value& a)

    (this may change so that the derived classes simply attempt the
    conversion and return 0 if they are unable to do anything).

  * The int argument has been removed from the

      octave_value (octave_value *, int)

    constructor.  You must increment the reference count (if needed) of
    the derived class, then construct the octave_value object.  For
    example, in a derived class method, you can return an octave_value
    object containing a copy of the current object using

      count++;
      return octave_value (this);

    (this may change because it seems complex and likely to cause
    confusion and trouble).

  * The get_rep function now returns a reference to an octave_base_value
    object instead of a reference to an octave_value object.  All uses
    must change.

Perhaps with a little more cleaning up, this will be a good change to
make.  It would also be a good time to fully document the octave_value
and octave_base_value classes and remove any unnecessary functions.

jwe

Reply | Threaded
Open this post in threaded view
|

Re: Octave_value class hierachy

Stéfan van der Walt
John,

Since you're at it, can you please add one line comments to these
functions to indicate what they do?  When I read this code for the
first time I found it a bit daunting, and I think even very crude
comments will make it a lot simpler to understand.

Stéfan

On Thu, Nov 17, 2005 at 01:03:00AM -0500, John W. Eaton wrote:

> On 16-Nov-2005, John W. Eaton wrote:
>
> | We would need to make the following changes:
> |
> |   * octave_value::rep becomes a pointer to octave_base_value instead of
> |     octave_value
> |
> |   * The octave_base_value class is no longer derived from octave_value.
> |
> |   * All other value classes must be derived from octave_base_value (I
> |     think this is already true, but it would become a requirement).
> |
> |   * Functions in the octave_value class would not need to be declared
> |     virtual, but the functions in octave_base_value would be virtual.
> |
> |   * In addition to changing the type of octave_value::rep, we would
> |     also have
> |
> |       typedef octave_base_value * (*type_conv_fcn) (const octave_value&);
> |       octave_value (octave_base_value *new_rep, int count = 1);
> |       octave_base_value *clone (void) const;
> |       octave_base_value *empty_clone (void) const;
> |       octave_base_value *try_narrowing_conversion (void);
> |       octave_base_value *internal_rep (void) const;
> |
> |     in the octave_value class instead of
> |
> |       typedef octave_value * (*type_conv_fcn) (const octave_value&);
> |       octave_value (octave_value *new_rep, int count = 1);
> |       virtual octave_value *clone (void) const;
> |       virtual octave_value *empty_clone (void) const;
> |       virtual octave_value *try_narrowing_conversion (void);
> |       octave_value *internal_rep (void) const;
> |
> |     and the nil_rep function could be moved to the octave_base_value
> |     class.  These changes would affect all derived types, but should
> |     only require a change in the declaration, with no change in the
> |     way the functions actually work.
> |
> |   * There would be no need for the octave_xvalue struct (which is just
> |     confusing anyway).
> |
> |   * The rep could be assumed to always be non-null.
> |
> |   * As Jens suggested, we could move count to the octave_base_value
> |     class.
>
> OK, I spent some time making this change (not checked in yet; I may
> put it on a separate branch).  It turned up some bugs (for example,
> inappropriately casting an octave_value object to a derived type) and
> some questionable design (the numeric_conversion_function method, for
> one).