Re: rc/ov.h:166: warning: 'octave_value::rep' should be initialized in the member initialization list

[Jaroslav Hajek]

On Fri, Jan 14, 2011 at 7:25 PM, CdeMills <address@hidden> wrote:
>
> Could someone please enlighten my weak comprehension of C++ with regard to
> ov.h ?
>
> Line 166:
> octave_value (void)
>    {
>      static octave_base_value nil_rep;
>      rep = &nil_rep;
>      rep->count++;
>    }
>
> Line 1174:
> protected:
>
>  // The real representation.
>  octave_base_value *rep;
>
> It looks like an implementation of reference counting and smart pointer:
>
>  ~octave_value (void)
>  {
>    if (--rep->count == 0)
>      delete rep;
>  }
>
> But why this static declaration of some object inside the ctor ? Unique for
> all objects of this class ? Equivalent to a closure ?
>

That was an optimization. If you had the default ctor instead do new
octave_base_value(), it would be way slower and consume more memory. I
vaguely recall this cut down the time to construct a big cell array by
factor of 5 or so.
Many other refcounted classes in Octave use this optimization as well.

> Moreover, as count is an octave_idx_type, which is an int, shouldn't case
> where count < 0 be detected and flagged  ? They should not occur, but
> better sure than sorry.

Technically, yes, this can occur in case of overflow. And it's not
just this place, there are many others. It should be technically
impossible to trigger this from the interpreter, however. dim_vector
won't let you construct an array whose size overflows octave_idx_type
(see dim_vector::dim_max and dim_vector::safe_numel). Creating several
large cell arrays from within the interpreter doesn't mind, because
Cell::resize_fill_value will create a separate fill value for each of
them. There should be a big fat warning "DON'T ever change this to a
static value" in that method :D
So the only way to shoot yourself in the foot is internally creating
several large uninitialized Array<octave_value> instances at once.
Hopefully we don't do that.

But in general, I agree that this is kind of fragile and deserves more
checking. But please also understand that these constructors (and
associated assignment ops, copy ctors and dtors) are speed-critical
and none of that "safety first, performance second" crap should be
applied here, any non-cosmetic changes in this stuff should absolutely
come with measurements of performance impact and should be discussed
first.