what are the problems with this piece of code?

[__PPS__]

Hello, I'm a university student and I'm preparing for my final today.
I'm reading course notes, I found completely strange piece of code. It
makes me laugh, I think the teacher needs to prepare herself for this
course.
so I ask for your point of view.
here's the piece of code:

Memory management by client!!
// Listing 9.4. Memory management by client rather than by server
object
class Name{
public:
char *contents; // PUBLIC pointer to dynamic memory
Name (char* name); // or Name (char name []);
void show_name();
~Name(); // destructor eliminates memory leak
};
Name::Name(char* name)// conversion constructor
....


Name::~Name(){ // destructor
delete contents; // it deletes heap memory, not the pointer
}

void Client(){
Name n1("Jones"); // conversion constructor is called
Name *p = (Name*) malloc(sizeof(Name)); // no constructor is called
p->contents = new char[strlen("Smith")+1]; // allocate dynamic memory
if (p->contents == NULL){ // 'new' was not successful
cout << "Out of memory\n"; exit(1); // give up
}
strcpy(p->contents, "Smith"); // 'new' was successful
n1.show_name(); p->show_name(); // use the objects
delete p->contents; // avoid memory leak
free (p); // notice the sequence of actions
}
// p is deleted, destructor for object n1 is called

(c) AISHY AMER CONCORDIA UNIV, ECE

 

[Victor Bazarov]

Hello, I'm a university student and I'm preparing for my final today.
I'm reading course notes, I found completely strange piece of code. It
makes me laugh, I think the teacher needs to prepare herself for this
course.

Prepare? What do you mean?

so I ask for your point of view.
here's the piece of code:

Memory management by client!!
// Listing 9.4. Memory management by client rather than by server
object
class Name{
public:
char *contents; // PUBLIC pointer to dynamic memory
Name (char* name); // or Name (char name []);
void show_name();
~Name(); // destructor eliminates memory leak

The first and foremost problem here is the absence of a copy-c-tor and
the copy assignment op. That's a violation of "the Rule of Three".

Of course, public data is bad.

Improvements could be made to the interface. The c-tor should take
a pointer to const char. The 'show_name' member should take an argument
(the stream to print out to), and this member should probably be 'const'.

};
Name::Name(char* name)// conversion constructor
...


Name::~Name(){ // destructor
delete contents; // it deletes heap memory, not the pointer

I think it's a bug because it should likely be

delete[] contents;

(although it is unknown how the 'contents' pointer is obtained.

}

void Client(){
Name n1("Jones"); // conversion constructor is called
Name *p = (Name*) malloc(sizeof(Name)); // no constructor is called
p->contents = new char[strlen("Smith")+1]; // allocate dynamic memory
if (p->contents == NULL){ // 'new' was not successful

'new' or 'new[]' does not actually return NULL upon a failure. It throws.

cout << "Out of memory\n"; exit(1); // give up
}
strcpy(p->contents, "Smith"); // 'new' was successful
n1.show_name(); p->show_name(); // use the objects
delete p->contents; // avoid memory leak

Must be

delete[] p->contents;

free (p); // notice the sequence of actions
}
// p is deleted, destructor for object n1 is called

(c) AISHY AMER CONCORDIA UNIV, ECE

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
What's that? Is that the school you're attending?

All in all, yes, that piece of code reminds me of something I've seen in
a bad dream after going over some amateurish C++ code written by some C
programmers. And I still have to maintain code similar to that.
Hopefully, after I am done with it, whoever comes next won't have to do
much. I can only hope.

V

 

[Bob Hairgrove]

[snip]

I think that was really the assignment ... to write down all the
problems this code has. You just did (some of) the homework for him
(or her)!

 

[Victor Bazarov]

[snip]

I think that was really the assignment ... to write down all the
problems this code has. You just did (some of) the homework for him
(or her)!

Oh well... I guess I'm getting old or maybe just feeling charitable
today.

 

[__PPS__]

I think that was really the assignment ... to write down all the

problems this code has. You just did (some of) the homework for him
(or her)!

Him, not her.
what's the dete today?? I gues, you could deduce that assignment time
is over, for most of the people this semester has already finished ...
anyways, you were not right, I know well what problems this code had,
the problem is that the teacher never listens to me, so I wanted to
give her links to this messages, maybe she would change her opinoin.
You may search for __PPS__ on gougle.groups to see other similar posts
about the code we had on exams etc. Still not convinced that it's not a
homework? that piece of code comes from this lecture presentation.
http://www.ece.concordia.ca/~amer/teach/coen244/notes/topic03.pdf
I guess you didn't believe that this code was really "production code"
of my teacher and not an example of bad code...


now I have another question.
basicly, the first one was a couple of hours before the final exam when
I wanetd to see her notes, cause I new that she will ask questions that
are covered in her lectures....
so, I came back from exam, and I have a couple of questions about it...

basicly, she requires always separate "declaration from implementation"

in her speak that means that for all classes we have to write .cpp &
..hpp files (exam is on paper)
there was a case when I had to write class student, that has a char *
m_name, initilizes memory for the students name & frees it on
destruction. Then I needed to write Undergraduate student class that
inherets from Student, plus it has linked list of courses that student
is registered for.
...basicly, I asked her is it ok to at least write implementation for
empty functions or constructors inside class definition (to write
"Undergraduate::Undergraduate" for me it takes almost an entire line on
paper - i write BIG )
she said no, it's not good style, you'll lose marks etc... still, I
left everything as I did (I asked after it was done). So, is there any
guidelines for writing these empty functions or whatever... from c++
point of view, is my code broken somehow because I did what I did?!
personally, i really don't like when I use some libraries for which
half of .cpp files contain autogenerated (or not autogenerated) empty
functions; I never do it myself like this. (I was asked to write
default constructor, I know it's ok to omit it when it's empty, but it
was clearly written that we had to!)

another question thing... she has strange style of making questions,
she has something on her mind and makes a question, but it's absolutely
makes no sence to me. In all assignments and tests that I did more than
half problems are like this. Here I'll write from my memory a simple
question that shows that:
////
Assume you have created class MyClass and implemented it correctly
(with copy & default constructors defined). This class has a member
AddBal, and here's the implementation of this member. What are the
errors with this code. Is it usefull?
MyClass MyClass::AddBal(MyClass a, MyClass b){
MyClass tmp;
tmp.balance = a.balance + b.balance;
return tmp;
}
////
that's it, all the names of classes variables and methods are
preserved, don't ask me questions what's that etc... I have no idea
what she had in her had when she wrote that piece of code
basicly, I wrote that this code will compile and execute correctly
provided there's no problem in all the dependent code. I wrote that
it's not possible to determine usefullness of this piece without
knowing anything about MyClass...
but, I'm sure she meant that it had to be this->balance += a.balance +
b.balance... or somethig like this
....

other than that, in most of the questions in annotation to problems she
says not to forget to check if any memory is reserved or not etc...
off course, intentionally I forgot to do that

 

[Howard]

MyClass MyClass::AddBal(MyClass a, MyClass b){
MyClass tmp;
tmp.balance = a.balance + b.balance;
return tmp;
}
////
that's it, all the names of classes variables and methods are
preserved, don't ask me questions what's that etc... I have no idea
what she had in her had when she wrote that piece of code
basicly, I wrote that this code will compile and execute correctly
provided there's no problem in all the dependent code. I wrote that
it's not possible to determine usefullness of this piece without
knowing anything about MyClass...
but, I'm sure she meant that it had to be this->balance += a.balance +
b.balance... or somethig like this
...

Really, your answer to her question was really as useless as the code.

If you have no idea why she wrote that code, ask her. If you don't know
what's wrong with the code, but you were expected to while taking the
course, I'd say you missed something, either because you skipped classes or
didn't understand her lectures or notes, or didn't study.

In any case, why don't you at least make an educated guess as to what you
think is wrong with the code above? If the course is truly over, then you
should know by now. If not, get a good book and study it.

Think about what each part of the code does, and whether there's any
problems or inefficiencies doing it that way. Then consider some code that
might need to use this code, and what the implications of doing so would be.
Could anything have been done better? (Your own observation that you're
"sure she meant it had to be..." is a start. Ask yourself why you said that.
Go from there.)

-Howard

 

[__PPS__]

makes me laugh, I think the teacher needs to prepare herself for this

Prepare? What do you mean?

I mean she doesn' know c++. She needs to learn it before teaching



You identified all the bugs I was thinking about, but what about the
line about casting allocated memory to a Name. It surly works since we
know what's there inside of Name, but is it really valid code from c++
point of view. That's probably the ugliest part in my opinion... (plus
plain memory leak from using delete instead of delete[])

p->contents = new char[strlen("Smith")+1]; // allocate dynamic memory
if (p->contents == NULL){ // 'new' was not successful

'new' or 'new[]' does not actually return NULL upon a failure. It throws.

Yeah, all over her code she checks it not to be NULL. I think she wants
to make sure that it will not be NULL in any case ...or plainly, she
doesn't know that new doesn't return NULLs.

cout << "Out of memory\n"; exit(1); // give up
}
strcpy(p->contents, "Smith"); // 'new' was successful
n1.show_name(); p->show_name(); // use the objects
delete p->contents; // avoid memory leak

Must be

delete[] p->contents;

free (p); // notice the sequence of actions
}
// p is deleted, destructor for object n1 is called

(c) AISHY AMER CONCORDIA UNIV, ECE

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
What's that? Is that the school you're attending?

That's how I copy-pasted it. Yes, that's her name and my university.
Besides, I googled out a nice page - proffesor raitings from many
schools in north america. My teacher gets a very low grade from
students, most of people from other classes share my point of view.
http://www.ratemyprofessors.ca/ShowRatings.jsp?tid=117439

 

[__PPS__]

Really, your answer to her question was really as useless as the code.

If you have no idea why she wrote that code, ask her. If you don't know
what's wrong with the code, but you were expected to while taking the
course, I'd say you missed something, either because you skipped classes or
didn't understand her lectures or notes, or didn't study.

what are you talking about?!?! what lectures?!? did you see this code
and the question? It cannot have any relation to any lectures at all.
It's a beginners class to c++ (intro)
and yes, I didn't attend any lecture, or any of her class. In some of
my previous posts I asked some questions and people started to fix my
termins (private, public, protected scope)... that's what she teaches
us in class. Other than that, I know c++ for as long as 5 years, I have
no trouble in class, I don't need help with homework, I don't get marks
below A+/A.... I wrote her a letter saying that I know material for all
the course and asked if there's something I should do to skip it, to
get exemption... she replied me in one sentence, saying that's not her
bussines , in other classes teacher at least tell to come to their
office.... so, do you get this feeling that it's quite useless to ask
her questions?..

In any case, why don't you at least make an educated guess as to what you
think is wrong with the code above? If the course is truly over, then you
should know by now. If not, get a good book and study it.

Course is over, the part with MyClass is an exam question, don't know
if the answers will be posted. The first piece of code is copy-pasted
from her lecture slides.

The code with MyClass is absolutely ok and has no errors. Did you find
any, if yes, then I'd also recomend you to get a book ...

Think about what each part of the code does, and whether there's any
problems or inefficiencies doing it that way. Then consider some code that
might need to use this code, and what the implications of doing so would be.
Could anything have been done better? (Your own observation that you're
"sure she meant it had to be..." is a start. Ask yourself why you said that.
Go from there.)

Ok, as I see you mix the two pieces of code I posted. Which one are you
talking about?? Where do you tell me to look for problems?? First is
complete pile of bugs, second with MyClass has no problems at all as I
said. Do you mean that constant references would be a better choice for
this case, is that what you mean about inefficiencies? I feel that
references is something that she has little knowledge about, she rarely
uses them (and that's why she didn't put references in this example).
Anyways, what makes you think that using const MyClass & a, .. b would
be more efficient? Without knowing anything about MyClass you cannot
say that, the other point is that using pass by value is not a error,
but the questions asks for errors.

 

[BobR]

__PPS__ wrote in message ...

Anyways, what makes you think that using const MyClass & a, .. b would
be more efficient? Without knowing anything about MyClass you cannot
say that, the other point is that using pass by value is not a error,
but the questions asks for errors.

Just by looking at this code, the way I see it:

MyClass MyClass::AddBal(MyClass const &a, MyClass const &b){
MyClass tmp;
tmp.balance = a.balance + b.balance;
return tmp;
}

Now a user can see that you are not going to change MyClass a or b (in
AddBal()), and a temporary copy (a,b. not talking about 'tmp') does not have
to be created in the method (like 'by value').
I think that's what Howard was talking about (inefficiencies).

[ Not being an expert, corrections are welcome. ]

 

[Jim Langston]

Prepare? What do you mean?

I mean she doesn' know c++. She needs to learn it before teaching



You identified all the bugs I was thinking about, but what about the
line about casting allocated memory to a Name. It surly works since we
know what's there inside of Name, but is it really valid code from c++
point of view. That's probably the ugliest part in my opinion... (plus
plain memory leak from using delete instead of delete[])

malloc returns a void pointer. It's the way malloc works. It doesn't know
what you're going to be using the memory it just allocated for, so returns a
void pointer. Then you need to type cast the returned pointer to the type
you're going to be using it for to assign it.

Basically I wouldn't use a class like that. What this code is doing is
allocating the memory for a class using malloc which bypasses the
constructor completely. Then allocating the memory for the array in the
class and setting it directly. There can be many problems with this, the
biggest problem being, what happens when the constructor gets changed? What
happens where a new array pointer is added and initialized to NULL in the
constructor. This code doesn't account for that, so wouldn't do it.

I had a mess on my hands converting some C source code to C++ when I was
converting some structures to classes. Because this is how they allocated
the structures. Made a pointer to it, malloced the memory, did a memset to
0. When I started to introduce constructors I scratched my head a little
while to figure out why my constructors weren't being called.

Yes, it can be done this way. But, as I'm likely to say, just because you
can do something doesn't mean you should.

p->contents = new char[strlen("Smith")+1]; // allocate dynamic memory
if (p->contents == NULL){ // 'new' was not successful

'new' or 'new[]' does not actually return NULL upon a failure. It
throws.

Yeah, all over her code she checks it not to be NULL. I think she wants
to make sure that it will not be NULL in any case ...or plainly, she
doesn't know that new doesn't return NULLs.

cout << "Out of memory\n"; exit(1); // give up
}
strcpy(p->contents, "Smith"); // 'new' was successful
n1.show_name(); p->show_name(); // use the objects
delete p->contents; // avoid memory leak

Must be

delete[] p->contents;

free (p); // notice the sequence of actions
}
// p is deleted, destructor for object n1 is called

(c) AISHY AMER CONCORDIA UNIV, ECE

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
What's that? Is that the school you're attending?

That's how I copy-pasted it. Yes, that's her name and my university.
Besides, I googled out a nice page - proffesor raitings from many
schools in north america. My teacher gets a very low grade from
students, most of people from other classes share my point of view.
http://www.ratemyprofessors.ca/ShowRatings.jsp?tid=117439

All in all, yes, that piece of code reminds me of something I've seen in
a bad dream after going over some amateurish C++ code written by some C
programmers. And I still have to maintain code similar to that.
Hopefully, after I am done with it, whoever comes next won't have to do
much. I can only hope.

 

[deane_gavin]

You identified all the bugs I was thinking about, but what about the
line about casting allocated memory to a Name. It surly works since we
know what's there inside of Name, but is it really valid code from c++
point of view. That's probably the ugliest part in my opinion... (plus
plain memory leak from using delete instead of delete[])

It is ugly because all casting is ugly. However, a cast is necessary
here. Better to use a C++ style cast though. I *think*
static_cast<Name*> is correct, but it might have to be
reinterpret_cast<Name*>.

Using delete instead of delete [] on a pointer obtained from new []
doesn't (necessarily) cause a memory leak - it's undefined behaviour.

Gavin Deane

 

[Thomas J. Gritzan]

__PPS__ wrote in message ...



Just by looking at this code, the way I see it:

MyClass MyClass::AddBal(MyClass const &a, MyClass const &b){
MyClass tmp;
tmp.balance = a.balance + b.balance;
return tmp;
}

Now a user can see that you are not going to change MyClass a or b (in
AddBal()), and a temporary copy (a,b. not talking about 'tmp') does not have
to be created in the method (like 'by value').
I think that's what Howard was talking about (inefficiencies).

Additionally, the function should be made static since it does nothing
to *this pointer. Or change the semantics so that uses *this.

Thomas

 

[__PPS__]

It is ugly because all casting is ugly. However, a cast is necessary

here. Better to use a C++ style cast though. I *think*
static_cast<Name*> is correct, but it might have to be
reinterpret_cast<Name*>.

As far as I'm concerned it's not valid to convert void pointer to
pointer to some object unless this void pointer was previously obtained
from pointer to this object...

please, if some one *does* know for sure about this case, what does the
starndard c++ tells about that? In my opinion it's undefined behavior.
Am I correct?
Just don't say it's not good to do so etc... (that's obvious)

the only way to combine malloc'ed memory with (non-pod) objects is to
use placement new, that's what I think... Not very sure though if
that's a good idea to do this kind of stuff to pod objects...


overall, what do you think about teacher who puts such code for
studying by students. In my opinion, she at least needs to refresh her
knowledge and take some c++ courses from people who really know this
subject

 

[Jerry Coffin]

__PPS__ wrote:

[ ... ]

As far as I'm concerned it's not valid to convert void pointer to
pointer to some object unless this void pointer was previously obtained
from pointer to this object...

Not necessarily. If it's a pointer to one kind of POD struct, you can
convert (directly or indirectly) to a pointer to another POD struct, as
long as the two have a common initial sequence. Of course, after you do
so, you can only access the members within that common initial sequence
(with defined behavior).

You can also (of course) convert a pointer to void returned by
malloc/calloc into a pointer to a POD type.

[ ... ]

the only way to combine malloc'ed memory with (non-pod) objects is to
use placement new, that's what I think... Not very sure though if
that's a good idea to do this kind of stuff to pod objects...

I suspect you just neglected to mention it, rather than really not
knowing, but you can also overload new to allocate memory via malloc.

 

[__PPS__]

As far as I'm concerned it's not valid to convert void pointer to

Not necessarily. If it's a pointer to one kind of POD struct, you can
convert (directly or indirectly) to a pointer to another POD struct, as
long as the two have a common initial sequence. Of course, after you do
so, you can only access the members within that common initial sequence
(with defined behavior).

Basicly, converting malloc'ed memory to <Name *> is undefined since
Name is not pod.
and yes, it's ok to convert from pointer to A to pointer to B if B is a
base class of A etc... but what do you mean by common initial
sequence??

class X { int a; char b; void * c; };
class Y{ int a; char b; void *c; double d; char x;};

do you mean that a, b & c members of the 2 structs are common and could
be used.
I was thinking that compiler was free to pack for example Y::b and Y::x
to come one after another instead of leaving padding if it's needed....
ok, even if it did so, it wouldn't change anything

You can also (of course) convert a pointer to void returned by
malloc/calloc into a pointer to a POD type.

But class Name (see my initial question) isn't pod, it has a
constructor etc... isn't so?

[ ... ]

the only way to combine malloc'ed memory with (non-pod) objects is to
use placement new, that's what I think... Not very sure though if
that's a good idea to do this kind of stuff to pod objects...

I suspect you just neglected to mention it, rather than really not
knowing, but you can also overload new to allocate memory via malloc.

well, that's different for this case, the memory is first allocated via
malloc and then used. there's no operator new in this example. (new is
used but for something unrelated to the question); Off course, with
overloaded new you are free to do what ever you want.

 

[Jerry Coffin]

__PPS__ wrote:

[ ... ]

Basicly, converting malloc'ed memory to <Name *> is undefined since
Name is not pod.

Sounds right.

and yes, it's ok to convert from pointer to A to pointer to B if B is a
base class of A etc... but what do you mean by common initial
sequence??

class X { int a; char b; void * c; };
class Y{ int a; char b; void *c; double d; char x;};

do you mean that a, b & c members of the 2 structs are common and could
be used.

Yes. This is used primarily in the case of a union. E.g. given
definitions like:

struct A {
int a;
char b;
};

struct B {
int a;
char b;
long c;
};

union C {
A a;
B b;
};

C c;

you can do something like:

c.a.a = 1;

int x = C.b.a;

and still get defined results (x is guaranteed to be 1). I'd have to do
some looking through the standard to be sure this is also guaranteed
when they're not in a union, but I think it should be. Realistically, I
have a hard time imagining a compiler ensuring that A and B had
compatible layouts when in a union, but incompatible layouts otherwise.

I was thinking that compiler was free to pack for example Y::b and Y::x
to come one after another instead of leaving padding if it's needed....
ok, even if it did so, it wouldn't change anything


But class Name (see my initial question) isn't pod, it has a
constructor etc... isn't so?

That looks correct, yes. I should apologize -- I jumped into the middle
of the thread, and didn't read the whole thread first. I was commenting
on your statements as they stood by themselves, not really in the
context of this particular thread.

 

[Marcus Kwok]

Yes. This is used primarily in the case of a union. E.g. given
definitions like:

struct A {
int a;
char b;
};

struct B {
int a;
char b;
long c;
};

union C {
A a;
B b;
};

C c;

you can do something like:

c.a.a = 1;

int x = C.b.a;

'C' should be lower case.

and still get defined results (x is guaranteed to be 1).

Wow, I am surprised. I always thought that you were only allowed to
access a field of a union though the variable you used to assign it.

Your code (correcting the capitalization and wrapping those statements
in main()) compiled with no errors or warnings with both VC++ 7.1 and
Comeau Online; however, the following code also compiled with no errors
or warnings:


#include <iostream>

union A {
int i;
float f;
};

int main()
{
A a;
a.i = 5;

float f = a.f; // I think this line is suspect

std::cout << "f = " << f << '\n';

return 0;
}

// outputs "f = 7.00649e-045" on my machine


So is it actually legal to access members of a union like this, as long
as you are aware of issues with how the data is interpreted?

 

[Jerry Coffin]

Marcus Kwok wrote:

[ ... ]

Your code (correcting the capitalization and wrapping those statements
in main()) compiled with no errors or warnings with both VC++ 7.1 and
Comeau Online; however, the following code also compiled with no errors
or warnings:

Right -- the code I posted has defined behavior.The code you added has
undefined behavior, but I don't know of any compiler that will warn you
about it.

[ ... ]

So is it actually legal to access members of a union like this, as long
as you are aware of issues with how the data is interpreted?

According to the standard (9.2/16), "If a POD-union contains two or
more POD-structs, it is permitted to inspect the common initial part of
any of them. Two POD-structs share a common initial sequence if
corresponding members have layout-compatible types (and, for
bit-fields, the same widths) for a sequence of one or more initial
members."

So, the behavior is defined ONLY for members that have essentially
identical types, that are only preceded by members that also have
essentially identical types ("essentially identical" in this case means
you're allowed a few minor variations like different enumerations as
long as they have the same underlying type).

 

[Marcus Kwok]

According to the standard (9.2/16), "If a POD-union contains two or
more POD-structs, it is permitted to inspect the common initial part of
any of them. Two POD-structs share a common initial sequence if
corresponding members have layout-compatible types (and, for
bit-fields, the same widths) for a sequence of one or more initial
members."

So, the behavior is defined ONLY for members that have essentially
identical types, that are only preceded by members that also have
essentially identical types ("essentially identical" in this case means
you're allowed a few minor variations like different enumerations as
long as they have the same underlying type).

I see, thank you.