what are the problems with this piece of code?

Discussion in 'C++' started by __PPS__, Dec 19, 2005.

  1. __PPS__

    __PPS__ Guest

    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
     
    __PPS__, Dec 19, 2005
    #1
    1. Advertisements

  2. Prepare? What do you mean?
    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'.
    I think it's a bug because it should likely be

    delete[] contents;

    (although it is unknown how the 'contents' pointer is obtained.
    'new' or 'new[]' does not actually return NULL upon a failure. It throws.
    Must be

    delete[] p->contents;
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    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
     
    Victor Bazarov, Dec 19, 2005
    #2
    1. Advertisements

  3. [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)!
     
    Bob Hairgrove, Dec 19, 2005
    #3
  4. Oh well... I guess I'm getting old or maybe just feeling charitable
    today.
     
    Victor Bazarov, Dec 19, 2005
    #4
  5. __PPS__

    __PPS__ Guest

    I think that was really the assignment ... to write down all the
    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
     
    __PPS__, Dec 19, 2005
    #5
  6. __PPS__

    Howard Guest

    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
     
    Howard, Dec 20, 2005
    #6
  7. __PPS__

    __PPS__ Guest

    makes me laugh, I think the teacher needs to prepare herself for this
    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[])

    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.


    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__, Dec 20, 2005
    #7
  8. __PPS__

    __PPS__ Guest

    Really, your answer to her question was really as useless as the code.
    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?..

    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 ...
    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.
     
    __PPS__, Dec 20, 2005
    #8
  9. __PPS__

    BobR Guest

    __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).

    [ Not being an expert, corrections are welcome. ]
     
    BobR, Dec 20, 2005
    #9
  10. __PPS__

    Jim Langston Guest

    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.
     
    Jim Langston, Dec 20, 2005
    #10
  11. __PPS__

    deane_gavin Guest

    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
     
    deane_gavin, Dec 20, 2005
    #11
  12. Additionally, the function should be made static since it does nothing
    to *this pointer. Or change the semantics so that uses *this.

    Thomas
     
    Thomas J. Gritzan, Dec 20, 2005
    #12
  13. __PPS__

    __PPS__ Guest

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

    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
     
    __PPS__, Dec 20, 2005
    #13
  14. __PPS__

    Jerry Coffin Guest

    __PPS__ wrote:

    [ ... ]
    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.

    [ ... ]
    I suspect you just neglected to mention it, rather than really not
    knowing, but you can also overload new to allocate memory via malloc.
     
    Jerry Coffin, Dec 21, 2005
    #14
  15. __PPS__

    __PPS__ Guest

    As far as I'm concerned it's not valid to convert void pointer to
    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 :)
    But class Name (see my initial question) isn't pod, it has a
    constructor etc... isn't so?
    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.
     
    __PPS__, Dec 22, 2005
    #15
  16. __PPS__

    Jerry Coffin Guest

    __PPS__ wrote:

    [ ... ]
    Sounds right.
    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.
    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.
     
    Jerry Coffin, Dec 22, 2005
    #16
  17. __PPS__

    Marcus Kwok Guest

    'C' should be lower case.
    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?
     
    Marcus Kwok, Dec 22, 2005
    #17
  18. __PPS__

    Jerry Coffin Guest

    Marcus Kwok wrote:

    [ ... ]
    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.

    [ ... ]
    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).
     
    Jerry Coffin, Dec 23, 2005
    #18
  19. __PPS__

    Marcus Kwok Guest

    I see, thank you.
     
    Marcus Kwok, Dec 25, 2005
    #19
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.