what is difference between these?

Question

I have this function which should copy a node in a linked list(not first of it)

struct Node {
    char* data;
    Node* next;
};

void Insert(Node*head, int index, char* data) {//find a is function to find needed position
    Node* temp = find(head, index);
    Node* t = (Node*)malloc(sizeof(Node));
    t->data = (char*)malloc(100);//where the problem is //line 4
    strcpy(t->data, data);
    t->next = temp->next;
    temp->next = t;
}

it will work well if line 4 is in my code. I have read this question:

crash-or-segmentation-fault-when-data-is-copied-scanned-read-to-an-uninitializ

so I know pointer cannot contain any data and I cannot copy/store data into a pointer. So, as you see I allocated memory for it first and then placed data in it, otherwise my program will crash.

But then I used this: t->data = data; and it worked, so I want to know: why when I use strcpy like this strcpy(t->data, data);, I need to allocate memory for t->data first, otherwise my program will crash; but this t->data = data; will work well, without the requirement to allocate memory?

Can you explain this to me?

PS:casting malloc is because of using c++ compiler.

Adrian is correct. Also, you can use: `t->data = strdup(data);` As it is, `100` is _hardwired_. If `data` has more than 100 chars, you're not allocating sufficient space. — Craig Estey, Mar 17 '20 at 20:07
@AdrianMole sorry .actually that was another question I asked and I was told this is wrong. I will edit it.thanks. — hanie, Mar 17 '20 at 20:08
Please post all the code needed to reproduce. What is `Node`? How is `data` memory managed? `it worked` what worked exactly? [Do not cast result of malloc](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). Please post a [MCVE]. — KamilCuk, Mar 17 '20 at 20:10
Normally you need to provide [A Minimal, Complete, and Verifiable Example (MCVE)](http://stackoverflow.com/help/mcve) -- however here the allocation and other errors are obvious. — David C. Rankin, Mar 17 '20 at 20:23
@hanie Are you using a C++ compiler? As C code, `Node` is not defined. — chux - Reinstate Monica, Mar 17 '20 at 20:25
@chux-ReinstateMonica yes right now I'm using c++ compiler(that is casting reason) — hanie, Mar 17 '20 at 20:27
@hanie Code posted is not valid C code - it does not compile, yet question is tagged C. It makes more sense to use a C compiler with C code that another language compiler. For a C++ question, the best answer differs. — chux - Reinstate Monica, Mar 17 '20 at 21:58

score 3 · Accepted Answer · edited Mar 17 '20 at 20:38

3

When you use the code t->data = data you are not copying any data to your node! All you are doing is make the node's data member point to the data that the function's argument also points to - so, if you later change that data, then you will also change the node's data. This is probably not what you intend! For example, if you call the function several times from 'the outside' and use the same variable to pass the data in, then each of your added nodes will have a data member pointing to the same piece of data.

If you actually want to copy the data from the argument to your new node (as your call to strcpy does), then you must first allocate storage space for it, using (in your code) the malloc function.

But, as mentioned in the comments, the strdup function is much more convenient here: this both allocates the (exact required amount of) memory and copies the data in one fell swoop:

t->data = strdup(data);

Note: The memory allocated by strdup needs to be released (with a call to free) when you're done with it, in just the same way as memory allocated by malloc.

edited Mar 17 '20 at 20:38

Chris Dodd

119,907
13
134
226

answered Mar 17 '20 at 20:12

Adrian Mole

49,934
160
51
83

Thanks for link but I use c++ compiler that is why I cast `malloc` ,and thanks for answer since my data was constant I didn't face this problem .Now I know how wrong is this method. – hanie Mar 17 '20 at 20:24
1

@hanie OK - I've edited out the casting link, and I have re-tagged your question as C++. – Adrian Mole Mar 17 '20 at 20:28
I'm coding c (in c++ compiler) and I don't have enough knowledge about c++ ,is my code right in c++? – hanie Mar 17 '20 at 20:32
@hanie It's syntactically correct (and will work) as C++ code! However, there are more modern ways to achieve this sort of thing since (about) C++11 - using standard template libraries (std::string, for example). – Adrian Mole Mar 17 '20 at 20:34

score 2 · Answer 2 · answered Mar 17 '20 at 20:12

Code only allocated for a size of a pointer when the size of the referenced object is needed.

// Node* t = (Node*)malloc(sizeof(Node*));
Node* t = (Node*)malloc(sizeof(Node));

Even better is to size to referenced object rather than the type. Cast not needed either.

Node* t = malloc(sizeof *t);

String allocation is suspicious too. Rather than a fixed 100, I'd expect an allocation to the size of the string.

//t->data = (char*)malloc(100);
//strcpy(t->data, data);
size_t len = string(data);
t->data = malloc(len + 1);
strcpy(t->data, data);

Robust code would check for allocation errors.

Node* t = malloc(sizeof *t);
if (t == NULL) Handle_OutOfMemory();

what is difference between these?

2 Answers2

Linked