newNode broken. Cannot figure whats wrong?

Question

I changed the code, moved the memcpy into the if statment but i still get the same error.

** * Returns a new linked list node filled in with the given order, The function * allocates a new order and copy the values stored in data then allocate a * linked list node. If you are implementing this function make sure that you * duplicate, as the original data may be modified by the calling function. */

struct order 
{
        int id;
        char side;
        int quantity;
        double price;
};

struct onode 
{
        struct order* data;
        struct onode* next;
        struct onode* prev;
};


    struct onode* newNode (struct order* data)
    {
        struct order* dataValue  = (struct order*) malloc(sizeof(struct order));
        struct onode* linkedlist = (struct onode*) malloc(sizeof(struct onode));


        if(data != NULL)
        {
            linkedlist ->data = dataValue;
            memcpy(dataValue, data, sizeof(*dataValue));
        }

        else
        {
            return NULL;
        }

        linkedlist->prev = NULL;
        linkedlist->next = NULL;

        return linkedlist;

    }

Can you provide some same input/output? How does it fail? Segfault/crash? Bad ordering? — Cloud, Mar 11 '13 at 17:09
On a side note, change calls like this; `(struct order*) malloc(sizeof(struct order));` to this: `malloc(sizeof(*dataValue));` — Ed S., Mar 11 '13 at 17:11
so i moved the memcpy into the if statment (if!=NULL) but i still get the error saying newNode has crashed. — Eciliptus, Mar 11 '13 at 17:15
@Dogbert this is the error i get Test failed: ---------------------------------------------------------------------- Your newNode seems to have crashed! With that broken, we can't test the rest of your code. You must fix it first! — Eciliptus, Mar 11 '13 at 17:16
@EdS. could you explain why i need to change it to *dataValue other then using order? — Eciliptus, Mar 11 '13 at 17:20
Good question. You don't *need* to, your version is correct in that regard, but what happens if you change the type of the variable? You now have to change the allocation as well. However, if you use `*dataValue` you don't; the compiler will always just use the type of `*dataValue`. It is the idiomatic way to allocate memory in C. As for the removal of the cast, see [this thread](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). If you can't remove the cast because the compiler barks at you then you are using a C++ compiler, not C. — Ed S., Mar 11 '13 at 18:00

Mike · Accepted Answer · 2013-03-11T17:39:24.643

1

The problem is not in the code you're showing. When calling it as such:

struct onode * mylist;
struct order * data = malloc(sizeof(struct order));
data->id = 4;
data->price = 20.11;
data->quantity = 3;
data->side = 'a';
mylist = newNode(data);

The list mylist is correctly generated with a node containing the values set in data. I suspect the code you're using to generate your list is not the same as what I'm showing above.

EDIT: To your "error"

Test failed: ----------------------------------------------------------------------
Your newNode seems to have crashed! With that broken, we can't test the rest of your code.
You must fix it first!

That's not an error anyone here can debug. Whatever tool your school is using to validate your code is expecting something different than what you provided. I would ask a TA about that if you don't have any other documentation.

EDIT2: You don't need to worry about coping values one at a time because of your memcpy():

memcpy(dataValue, data, sizeof(*dataValue));

This will take everything you set in data and put those values in the correct place in dataValue. In my example (above) I set 4 for the id, 3 for the quantity etc. After that memcpy() call, now both dataValue and data will have all those same values set. It coppied the values in data to dataValue. Get it?

edited Mar 11 '13 at 17:39

answered Mar 11 '13 at 17:23

Mike

47,263
29
113
177

it makes sense. I need to allocate mylist and data correctly. i have not done it anywhere. – Eciliptus Mar 11 '13 at 17:28
just a quick question, will the following be correct? data -> id = linkedlist->data->id ? since I the id's have to match – Eciliptus Mar 11 '13 at 17:30
@Eciliptus - uh.. those types match, yes. If you're talking about doing something like that in your `newNode()` function that logic seems a little backwards, (since you're passing in data you wouldn't want to overwrite it) – Mike Mar 11 '13 at 17:32
this is how i am approaching it. I need to copy the data so i use memcpy for that. I am still a little troubled on how can i get the id, price and everything else to be stored in the linkedlist. I thought doing that ( data -> id = linkedlist ->data->id) will take care of that. But seems its not rite. – Eciliptus Mar 11 '13 at 17:35
i highly appreciate the help, could you guide me a bit more on how can i achieve this function. I dont fully understand it and that all i want to do. I dont want the answer but the understanding on how to achieve it. working with single struct was fine but double struct confuse me – Eciliptus Mar 11 '13 at 17:38
@Eciliptus - you're over thinking it. See my Edit2 above. `memcpy()` will copy the memory (values) from `data` to `dataValue`. So if you initialize `data` correctly you don't need to do a memberwise type transfer of data by hand – Mike Mar 11 '13 at 17:40

Neil Townsend · Answer 2 · 2013-03-11T17:47:43.380

0

A few pointers:

You should return if data is NULL right at the start of the routine to avoid allocating memory you then will ignore if data is NULL.
The memcpy line is correct, but it is probably clearer syntax to change the sizeof so it's clear how much memory is being copied:

memcpy(dataValue, data, sizeof(struct order));
You don't actually put the set up data in the onode, I suspect you need the line:

linkedList->data = dataValue;

As you ask in your comment, you could also fill out the new struct order:

dataValue->id = data->id // etc.

And you can then do comparisions:

if (data->id == linkedList->data->id) {
    /* It matches */
}

edited Mar 11 '13 at 17:47

answered Mar 11 '13 at 17:30

Neil Townsend

6,024
5
35
52

if i do data -> id = linkedlist->data->id Does this mean that both the id's match? do i need to do this or anything similar to that? thank you for all the help – Eciliptus Mar 11 '13 at 17:32
1

Point 2 is incorrect. `sizeof(struct order)` and `sizeof(*dataValue)` will result in the same size – Mike Mar 11 '13 at 17:35

score -1 · Answer 3 · answered Mar 11 '13 at 17:21

You are calling memcpy with data as the source but in your next statement, you test for data being not null. This tells me a null data pointer is acceptable in which case memcpy will crash. You probably need to rearrange the order of statements here.

BTW, have you run this through your debugger yet?

newNode broken. Cannot figure whats wrong?

3 Answers3