Pitfalls and problems in this code?

[Bryan]

I hate it when people reinvent the wheel. This is some legacy code that
I believe may be causing some memory issues. I think there may be a
problem in the way that the CVector class is kill_resize'd or assigned.
Any thoughts?

class CVector
{
// types:
public:
typedef double data_type;

// Constructors
public:
CVector();
CVector(long source_length);
CVector(long source_length, const data_type* const source_data);
CVector(const CVector &fil);
virtual ~CVector();

// Operators
public:
CVector& operator= (const CVector& source);

// Functions
public:
data_type* begin() const { return data; };
data_type* end() const { return data + length; };
long size() const { return length; };
bool empty() const { return size() == 0; }

// set new vector length _without_ copying
void kill_resize (long source_length);

data_type* data;

// Member variables:
protected:
long length;
};


// Constructors:
CVector::CVector(): data(0), length(0) {}

CVector::CVector(long source_length):
length(source_length), data(new data_type[source_length]) {}

CVector::CVector(long source_length, const data_type* const source_data):
length(source_length), data(new data_type[source_length])
{
if (data) memcpy(data, source_data, length * sizeof(data_type));
}

CVector::CVector(const CVector &v):
length(v.length), data(new data_type[v.length])
{
if (data) memcpy(data, v.data, length * sizeof(data_type));
}

// Destructor
CVector::~CVector()
{
if (data) delete[] data;
data = 0; // do not free again.
length = 0;
}

/////////////////////////////////////////////////////////////////////////////////////////

// Change the length of an existing vector WITHOUT COPYING

void CVector::kill_resize (long source_length)
{
if (data && (source_length != length))
{
delete[] data; // there was something in here
data = 0; // do not free again.
}

if (!data) data = new data_type[source_length]; // get memory
length = source_length;
}

/////////////////////////////////////////////////////////////////////////////////////////

CVector& CVector:perator= (const CVector &source)
{
if (source.data == NULL) {
if (data != NULL) {
delete[] data; // there was something in here
data = 0; // do not free again.
}
length = 0;

return *this;
}

if (data == source.data) {
return *this;
}

kill_resize (source.length);
if (source.data) memcpy(data, source.data, length * sizeof(data_type));

return *this;
}

 

[Victor Bazarov]

I hate it when people reinvent the wheel. This is some legacy code that
I believe may be causing some memory issues. I think there may be a
problem in the way that the CVector class is kill_resize'd or assigned.
Any thoughts?
Several.

class CVector
{
// types:
public:
typedef double data_type;

// Constructors
public:
CVector();
CVector(long source_length);
CVector(long source_length, const data_type* const source_data);

The second 'const' is superfluous.

CVector(const CVector &fil);
virtual ~CVector();

'virtual'? Do you expect this class to serve as a base class?

// Operators
public:
CVector& operator= (const CVector& source);

// Functions
public:
data_type* begin() const { return data; };
data_type* end() const { return data + length; };

Semicolons after a curly brace are superfluous. Drop 'em.

Are you sure you want to return a pointer to non-const objects for
the data of a [potentially] const CVector? I'd probably declare
those

data_type const* begin() const;
data_type const* end() const;

long size() const { return length; };
bool empty() const { return size() == 0; }

// set new vector length _without_ copying
void kill_resize (long source_length);

data_type* data;

Why is 'data' public?

// Member variables:
protected:
long length;
};


// Constructors:
CVector::CVector(): data(0), length(0) {}

CVector::CVector(long source_length):
length(source_length), data(new data_type[source_length]) {}

CVector::CVector(long source_length, const data_type* const source_data):
length(source_length), data(new data_type[source_length])
{
if (data) memcpy(data, source_data, length * sizeof(data_type));

'data' will never be 0 here unless you use 'new (nothrow) '. If you
don't (like here), an exception 'std::bad_alloc' is thrown if it can't
allocate.

It is generally better to use

std::copy(source_data, source_data + length, data);

instead of 'memcpy'. You will need those everywhere you use 'memcpy' as
soon as you convert your class to be a template (you're going that way,
aren't you?)

}

CVector::CVector(const CVector &v):
length(v.length), data(new data_type[v.length])
{
if (data) memcpy(data, v.data, length * sizeof(data_type));

Same sentiment as above. 'data' is never 0. Don't use 'memcpy'.

}

// Destructor
CVector::~CVector()
{
if (data) delete[] data;

There is no harm in deleting a null pointer.

data = 0; // do not free again.
length = 0;

There is no need to change members of an object being destructed. They
are on their way out.

}

/////////////////////////////////////////////////////////////////////////////////////////


// Change the length of an existing vector WITHOUT COPYING

void CVector::kill_resize (long source_length)
{
if (data && (source_length != length))
{
delete[] data; // there was something in here
data = 0; // do not free again.
}

if (!data) data = new data_type[source_length]; // get memory

For consistency's sake, you ought to check whether the 'data' is still
non-null here. You apparently don't. Well, you don't need to, of course.

length = source_length;
}

/////////////////////////////////////////////////////////////////////////////////////////


CVector& CVector:perator= (const CVector &source)
{
if (source.data == NULL) {
if (data != NULL) {
delete[] data; // there was something in here
data = 0; // do not free again.
}
length = 0;

return *this;
}

if (data == source.data) {
return *this;
}

kill_resize (source.length);
if (source.data) memcpy(data, source.data, length * sizeof(data_type));

return *this;
}

Well, I don't see anything particularly bad here, but there is still
a possibility of dissinchronization between 'data' and 'length'. I would
recommend adding something like

assert((data != 0) == (length > 0));

in every function -- that's a consistency check.

V

 

[Alf P. Steinbach]

* Bryan:

I hate it when people reinvent the wheel. This is some legacy code that
I believe may be causing some memory issues. I think there may be a
problem in the way that the CVector class is kill_resize'd or assigned.
Any thoughts?

class CVector
{
// types:
public:
typedef double data_type;

Seems this was aimed at later templatizing.

// Constructors
public:
CVector();
CVector(long source_length);
CVector(long source_length, const data_type* const source_data);
CVector(const CVector &fil);
virtual ~CVector();

// Operators
public:
CVector& operator= (const CVector& source);

// Functions
public:
data_type* begin() const { return data; };
data_type* end() const { return data + length; };

Destroys logical constness.

long size() const { return length; };
bool empty() const { return size() == 0; }

// set new vector length _without_ copying
void kill_resize (long source_length);

data_type* data;

Public data how nice.

// Member variables:
protected:
long length;

Seems inconsistent with the access for 'data'.

};


// Constructors:
CVector::CVector(): data(0), length(0) {}

CVector::CVector(long source_length):
length(source_length), data(new data_type[source_length]) {}

CVector::CVector(long source_length, const data_type* const source_data):
length(source_length), data(new data_type[source_length])
{
if (data) memcpy(data, source_data, length * sizeof(data_type));
}

The 'if' is superflous.

CVector::CVector(const CVector &v):
length(v.length), data(new data_type[v.length])
{
if (data) memcpy(data, v.data, length * sizeof(data_type));
}

The 'if' is superflous.

// Destructor
CVector::~CVector()
{
if (data) delete[] data;
data = 0; // do not free again.
length = 0;
}

/////////////////////////////////////////////////////////////////////////////////////////

// Change the length of an existing vector WITHOUT COPYING

void CVector::kill_resize (long source_length)
{
if (data && (source_length != length))
{
delete[] data; // there was something in here
data = 0; // do not free again.
}

if (!data) data = new data_type[source_length]; // get memory
length = source_length;
}

If data != 0 were made a class invariant (simply fix the default constructor)
the logic above would be equivalent to

if( source_length != length )
{
delete[] data; // there was something in here
data = new data_type[source_length]; // get memory
}
length = source_length;

which seems to be correct.

Note that kill_resize guarantees data != 0 on exit.

/////////////////////////////////////////////////////////////////////////////////////////

CVector& CVector:perator= (const CVector &source)
{
if (source.data == NULL) {
if (data != NULL) {
delete[] data; // there was something in here
data = 0; // do not free again.
}
length = 0;

return *this;
}

If data != 0 were made a class invariant (simply fix the default constructor)
the above would never happen and that code could be removed.

if (data == source.data) {
return *this;
}

This avoids self-assignment in a round-about way, OK.

kill_resize (source.length);
if (source.data) memcpy(data, source.data, length * sizeof(data_type));

The 'if' is superflous. The code at the top of this function guarantees
source.data != 0. And the call to kill_resize guarantees data != 0.

 

[int2str]

Destroys logical constness.

For our (mine) education, could you elaborate?

I'm unfamiliar with the term "logical constness" and do not understand
what is wrong with the code fragment.

Thanks,
Andre

 

[Victor Bazarov]

For our (mine) education, could you elaborate?

I'm unfamiliar with the term "logical constness" and do not understand
what is wrong with the code fragment.

class A {
int *pa;
public:
A(int a_) : pa(new int(a_)) {}
A(const A& aa) : pa(new int(*aa.pa)) {}
~A() { delete pa; }

int* get_a() const { return pa; } // BAD!
};

int main() {
A const a_const(42); // is 'a_const' actually const?
int* pi = a_const.get_a();
*pi = 73; // KABOOM!
// I just changed the "value" of a const object
}

Logical constness means that if I declare an object 'const', none of its
parts (including the contents of the dynamic memory it allocates) should
be changeable.

V

 

[Ron Natalie]

class A {
int *pa;
public:
A(int a_) : pa(new int(a_)) {}
A(const A& aa) : pa(new int(*aa.pa)) {}
~A() { delete pa; }

int* get_a() const { return pa; } // BAD!
};

int main() {
A const a_const(42); // is 'a_const' actually const?
int* pi = a_const.get_a();
*pi = 73; // KABOOM!
// I just changed the "value" of a const object
}

Logical constness means that if I declare an object 'const', none of its
parts (including the contents of the dynamic memory it allocates) should
be changeable.

You'll have to argue wehter *pa is part of the pobject or not.

 

[Victor Bazarov]

You'll have to argue wehter *pa is part of the pobject or not.

No, I won't have to argue, unless you make me argue. Read my definition
of logical constness. The OP asked what it was, I explained. That's the
definition both I and Alf used when claiming the semantical misgivings of
the OP's code. There can be another (your, for example, whatever it is)
definition of "logical constness". I've never claimed mine to be the only
true one.

V