StackOverflow2013

Note that there are some explanatory texts on larger screens.

plurals

PO
primarykey
Id
13003512
data
AcceptedAnswerId
0
AnswerCount
0
ClosedDate
CommentCount
3
CommunityOwnedDate
CreationDate
2012-10-22T00:13:33.970
FavoriteCount
0
LastActivityDate
2012-10-22T02:41:51.133
LastEditDate
2017-05-23T12:04:41.153
LastEditorUserId
-1
OwnerUserId
15168
ParentId
13003275
PostTypeId
2
Score
6
ViewCount
0
LastEditorDisplayName
text
Body
<h2>Code Analysis</h2> Let's dissect this a bit at a time: <pre><code>int function2(struct Person *list) { </code></pre> As <a href="https://stackoverflow.com/users/1428679/lserni">Iserni</a> noted in his <a href="https://stackoverflow.com/questions/13003275/malloc-struct-in-a-function-free-before-ending-program/13003302#13003302">answer</a>, this definition is inconsistent with your call to the function. I agree in general with his correction for the existing code (but I'm going to recommend modifying it shortly): <pre><code>int function2(struct Person *list, int *index) struct Person *prsn = malloc(sizeof(struct Person)); // !Why do we sometimes cast the malloc or not? // I sometimes get errors when I do, sometimes when I don't, // while the surrounding code is pretty much the same. </code></pre> It depends on whether you're in C or C++; in C++, you must cast the return value from <code>malloc()</code> (if you use <code>malloc()</code> at all; you usually shouldn't in C++). In C, the cast is optional. There is a school of thought that omitting the cast reveals errors that inserting the cast can hide. I don't subscribe to that; I believe that <code>malloc()</code> should have been declared via <code><stdlib.h></code> and the compiler should be warning if there is no declaration in scope, and if there's a declaration in scope, the cast can't cover up sins. The other possible problem is that you assign a pointer to a non-pointer; that would be a mistake that the compiler should be complaining about too. <pre><code> assert(prsn != NULL); </code></pre> This is not normally considered a sensible long-term method of handling memory allocation errors. <pre><code> // User input code goes here ... // Now to save the Person created strcpy(prsn->name, nameInput); prsn->age = ageInput; list[(*index)++] = *prsn; // !Why use the dereferencing *prsn here? </code></pre> Because: <ul> <li><code>list</code> is a <code>struct Person *</code>.</li> <li>Therefore <code>list[i]</code> is a <code>struct Person</code> (albeit you spelled <code>i</code> as <code>(*index)++</code>).</li> <li>Therefore you must assign a <code>struct Person</code> to it.</li> <li><code>prsn</code> is a <code>struct Person *</code>.</li> <li>Therefore <code>*prsn</code> is also a <code>struct Person</code>.</li> <li>Therefore the assignment is 'correct'.</li> <li>It also gives you the leak.</li> <li>You've overwritten the content of <code>list[i]</code> with the content of <code>*prsn</code>.</li> <li>You've not save the pointer to <code>prsn</code> anywhere.</li> <li>So you leak memory as you return from the function.</li> </ul> The surgery required to fix this is non-negligible: <pre><code>int function2(struct Person **item) ... *item = prsn; </code></pre> and you have to revise the call; I'll come back to that when dissecting <code>main()</code>. <pre><code> // why not directly prsn? Or is that saving the memory address and not very useful. } </code></pre> Your function is declared to return an <code>int</code> but you show no <code>return</code>. If you aren't going to return a value, declare the function as <code>void</code>, especially if you're going to ignore the return value as your code in <code>main()</code> does. Your last comment is mostly covered by the discussion above; saving the memory address is crucial to stopping the leak so it is very useful. <blockquote> And this is my main function: </blockquote> <pre><code>int main(int argc, char *argv[]) { struct Person personList[MAX_SIZE]; int index; </code></pre> Using an uninitialized variable is bad news. It is at best only accidentally zeroed. You can't afford to have random values in use as array indexes; initialize it explicitly. Also, we're going to need an array of pointers, not an array of structures: <pre><code> struct Person *personList[MAX_SIZE]; int index = 0; ...other code... function2(personList, &index); </code></pre> With the revised function: <pre><code> function2(&personList[index++]); </code></pre> This is preferable; it means that <code>function2()</code> doesn't need to know about the array; you just pass it the address of the pointer to which the allocated memory pointer should be assigned. This reduces the coupling between your <code>main()</code> function and <code>function2()</code>, which makes the code simpler all around. <pre><code> // Before closing, I want to free any mallocs I have done here. free() </code></pre> So you write: <pre><code> for (int i = 0; i < index; i++) free(personList[i]); </code></pre> This releases all the memory allocated. <pre><code> return 0; } </code></pre> I like to see an explicit return at the end of <code>main()</code>, even though C99 says it isn't 100% necessary. Make sure you are compiling with enough warnings enabled. If you're using GCC, then <code>gcc -Wall</code> should be the minimum level of compilation warning you run with (and you should have no warnings in your code when you do so). I run with more stringent warnings: <code>gcc -std=c99 -Wall -Wextra -Wmissing-prototypes -Wold-style-definition -Wstrict-prototypes -Wshadow</code> usually. You need to include <code>-O3</code> (or some level of optimization) to get some warnings. The stuff about prototypes reflects paranoia about an old code-base I work with which still has K&R function definitions around. <hr> <h2>Answering comments</h2> <blockquote> First question while I go over your details and try things out: is there a memory impact between an array of structs and an array of pointers? </blockquote> Yes, but it may not be what you're thinking of. If you use an array of structures, there's no need to allocate memory dynamic for the structures since the compiler's already allocated them for you. Since the purpose of the exercise is to use pointers and <code>malloc()</code>, it is better, therefore, to use pointers. In terms of space, there will be slightly more total memory used with the array of pointers (but there will be less memory leaked). <blockquote> I am trying to change my code to use an array of pointers. But using <code>function2(personList, &index);</code> to call <code>function2</code> now gives me the following warning: <code>incompatible pointer types passing 'struct Person *[512]' to parameter of type 'struct Person *'</code>. Is it OK if I write extra code in my main question to go into details? As a note, I'm trying to reference variables as much as possible, so as to not temporarily have the program copy data from function to function. </blockquote> The compiler is correct if you've not made all the changes. Your code using two arguments copies more data between functions than my code using one argument. <h3>Version 1</h3> The following program uses the proposed single-argument <code>function2()</code> to reduce the coupling between the <code>main()</code> function and <code>function2()</code>, simplifying both. This code compiles with no warnings under GCC 4.7.1 on Mac OS X 10.7.5 using the command line: <pre><code> gcc -O3 -g -std=c99 -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes \ -Wold-style-definition mem.c -o mem </code></pre> When run under <code>valgrind</code>, it comes up with no memory leaked. <pre><code>#include <assert.h> #include <stdio.h> #include <stdlib.h> #include <string.h> enum { MAX_INPUT = 28 }; enum { MAX_SIZE = 3 }; struct Person { char name[MAX_INPUT]; int age; }; static void function2(struct Person **list) { struct Person *prsn = malloc(sizeof(struct Person)); assert(prsn != NULL); char *nameInput = "This is my name"; int ageInput = 29; // Again! strcpy(prsn->name, nameInput); prsn->age = ageInput; *list = prsn; } int main(void) { struct Person *personList[MAX_SIZE]; int index = 0; function2(&personList[index++]); function2(&personList[index++]); function2(&personList[index++]); for (int i = 0; i < index; i++) free(personList[i]); return 0; } </code></pre> <h3>Version 2</h3> This keeps the two-argument version of <code>function2()</code> and has it do the counting that <code>main()</code> should be doing on its own. This program also compiles cleanly and runs cleanly under <code>valgrind</code>. <pre><code>#include <assert.h> #include <stdio.h> #include <stdlib.h> #include <string.h> enum { MAX_INPUT = 28 }; enum { MAX_SIZE = 3 }; struct Person { char name[MAX_INPUT]; int age; }; static void function2(struct Person **list, int *index) { struct Person *prsn = malloc(sizeof(struct Person)); assert(prsn != NULL); char *nameInput = "This is my name"; int ageInput = 29; // Again! strcpy(prsn->name, nameInput); prsn->age = ageInput; list[(*index)++] = prsn; } int main(void) { struct Person *personList[MAX_SIZE]; int index = 0; function2(personList, &index); function2(personList, &index); function2(personList, &index); for (int i = 0; i < index; i++) free(personList[i]); return 0; } </code></pre>
Tags
Title
singulars
PostAcceptedAnswerId
1. This table or related slice is empty.
PostParentId
1. POmalloc() struct in a function, free() before ending program
 singulars
 PostTypePostTypeId
 PTQuestion
PostTypePostTypeId
1. PTAnswer
UserLastEditorUserId
1. USCommunity
UserOwnerUserId
1. USJonathan Leffler
plurals
PostLinksPostIdRelatedPostId
1. This table or related slice is empty.
PostLinksRelatedPostIdPostId
1. This table or related slice is empty.
PostsAcceptedAnswerId
1. POmalloc() struct in a function, free() before ending program
 singulars
 PostTypePostTypeId
 PTQuestion
PostsParentIdCreationDate
1. This table or related slice is empty.
VotesPostIdCreationDate
1. VO
 singulars
 PostPostId
 PO
 UserUserId
 This table or related slice is empty.
 VoteTypeVoteTypeId
 VTUpMod
2. VO
 singulars
 PostPostId
 PO
 UserUserId
 This table or related slice is empty.
 VoteTypeVoteTypeId
 VTUpMod
3. VO
 singulars
 PostPostId
 PO
 UserUserId
 This table or related slice is empty.
 VoteTypeVoteTypeId
 VTUpMod
CommentsPostId

Querying!

Guidance

A row detail

Detail views are divided into sections. All the information in the data section comes from columns in the selected row. The other sections display data from other, related rows.

Related data can be related in a to-one or a to-many fashion. Captions of data related in a to-many fashion link to a list view showing a filtered view of the table.

Try moving around until you find a non-empty to-many entry and click on the label to get to one. You can move back to the root by clicking on the database name in the header.