1

I have a struct and a function that returns an instance of Foo defined as follows:

struct Foo
{
    int a;
    int* b;     
};

struct Foo makeFoo(int a, int bSize)
{
    struct Foo foo;
    foo.a = a;
    foo.b = malloc(sizeof(int) * bSize);
    for (int i = 0; i < bSize; ++i)
        foo.b[i] = i;

    return foo;
}

Initially, I thought foo is a local variable and it'll be gone whenmakeFoo returns, but from this question Is it safe to return a struct in C or C++?, I know that it's safe to do so.

Now my question is when will memory for foo be collected? Do I have to free its b member first?

Let's say I use makeFoo like this:

void barFunc()
{
    struct Foo foo = makeFoo(3, 10);

    printf("Foo.a = %d;\nFoo.b = [", foo.a);
    for (int i = 0; i < 10; ++i)
        printf("%d, ", foo.b[i]);

    printf("\n");
}

void main(int argc, char* argv)
{
    barFunc();  
}

When barFunc returns and I'm back in main, is memory for foo collected yet? Or do I have to call free(foo.b) at the end of barFunc?

Community
  • 1
  • 1
0x56794E
  • 20,883
  • 13
  • 42
  • 58
  • 2
    If you have a malloc, you need a free to release it. If you have no free, then it remains allocated. – Martin James Oct 20 '15 at 00:07
  • Will only the memory for `foo.b` remain or the entire struct? – 0x56794E Oct 20 '15 at 00:13
  • And also, is the behavior well defined? Or it depends? – 0x56794E Oct 20 '15 at 00:22
  • it is a poor programming practice to name an instance of a struct the same as the tag name of the struct, with only a capitalization difference. This kind of naming leads to mis-understandings and is a real headache when performing maintenance, say 6 months (or 6 years) later by you or another programmer – user3629249 Oct 20 '15 at 00:25
  • @user3629249 that's really a matter of taste. coding guidelines often say it's ok to have a variable of the same name as it's type. If you further defined that "complex" types should have *pascal case* and local variables should have *camel case*, this is what you end up with ... and it's well understood by everyone else following the same guidelines. –  Oct 20 '15 at 00:28
  • You have to `free` precisely what you `malloc`. That's the rule. At some point the same value you got back from `malloc` should be passed to `free` or your code has a leak. – David Schwartz Oct 20 '15 at 00:30
  • passing a struct, rather than a pointer to a struct is a poor programming practice as (not so much with a 8 byte struct) the passing of the struct is usually accomplished by the compiler reserving some memory, large enough to hold the struct, then calling memcpy() to copy the actual/original struct to the reserved memory, then calling memcpy() again to pass the struct back to the caller. I.E,. malloc a struct, then just pass pointers and at the end call free() on the struct. – user3629249 Oct 20 '15 at 00:35
  • @user3629249 this is like plain classes in [tag:c++] and structs in [tag:c#] work. Don't call it *bad practice* unconditionally. It's just important to keep in mind what you said (and I mentioned it as well in my answer). You don't want that for *large* objects, for obvious reasons. –  Oct 20 '15 at 00:38
  • It leaks like hell and main() shall return int. – wildplasser Oct 20 '15 at 00:38
  • @FelixPalmen, I have often had to perform maintenance on some code written by another person, often when that original programmer is long gone. I know, from the hard hand of experience what a poor programming practice separating names by only the capitalization does. I would prefer to point a (usually new/student) programmer into good programming habits. edit: yes, it is 'legal' C code to even use exactly the same spelling. Modern compilers can handle the name space cluttering. But I'm a human so need a bit more diversification to help assure no mis-understandings in code – user3629249 Oct 20 '15 at 00:42
  • @wildplasser it leaks a single allocated object. Although even this is *to much* in a long running process, I wouldn't call it "like hell". And yes, the `main` prototype is indeed wrong. –  Oct 20 '15 at 00:42
  • @user3629249 in our company, it's explicitly stated that having an instance variable or property of the same name as the type is ok, and there are good reasons for it (normally: not making up names that aren't semantically expressive -- indeed, not talking about "foo" here, but this is example code). Get better refactoring tools. –  Oct 20 '15 at 00:44
  • @FelixPalmen void functions are for void people. Java hipsters :-) – wildplasser Oct 20 '15 at 00:46
  • 1
    @user3629249 - Whether or not is poor programming practice is debatable. The behavior of returning a struct, however, is well-defined. – Andrew Henle Oct 20 '15 at 00:47
  • 1
    @wildplasser missed the target somewhat, I avoid java wherever I can ;) –  Oct 20 '15 at 00:48
  • Any memory leak is a sure sign of sloppy programming, When the project gets to be hundreds of source and header files, numerous executables, several different builds in the field and many different programmers. (as any large project (and even many small projects) illustrate; good programming practices must be followed. ( I personally really hate having to debug some others' sloppy code so I can debug mine.) and it is all too easy to miss a capitalization difference or noticing that some struct instance name is the same as the tag name. – user3629249 Oct 20 '15 at 00:58
  • 2
    @user3629249 please understand there's (nearly) no absolute "right" and "wrong" in coding. There are guidelines, best practices, and the like, and they exist for good reasons. And still, you will always find edge cases where it's better *not* to follow them. Take the example of "always free what you malloc". If you have a simple "pipes and filters" architecture and one of your filters needs to allocate a lot of memory for a chunk, but exits after processing it, a lot of `free()` calls would just be a waste in code size and execution time. –  Oct 20 '15 at 01:01

3 Answers3

3

Initially, I thought foo is a local variable and it'll be gone when makeFoo returns, but from this question Is it safe to return a struct in C or C++?, I know that it's safe to do so.

You do realize the local foo indeed is gone? returning a struct by value as you do here just copies its contents to the instance provided by the caller.(*)

But, of course, the contents include a pointer pointing to some memory you allocated with malloc(). So it has to be free()d later.

(*) This can be a good idea for small structs, depending on your needs, but always keep in mind the whole contents are copied -- it's definitely not what you want for a somehow large struct.

2

You have to call free(foo.b) by yourself before the foo variable in barFunc goes out of scope. This is not necessary in the makeFoo function because foo and the pointer to the allocated memory is copied to the caller, so it's all right.

Since you have a makeFoo function, it would be good practice to have a deleteFoo function too:

void deleteFoo(struct Foo *foo)
{
  free(foo->b);
}

void barFunc()
{
  struct Foo foo = makeFoo(3, 10);
  ...
  deleteFoo(&foo);
}
ChronoTrigger
  • 8,459
  • 1
  • 36
  • 57
  • No. See @Felix Palman's answer - returning the struct copies the contents of that struct, so the value of the pointer is passed to the caller, which can deal with calling `free()`. – Andrew Henle Oct 20 '15 at 00:44
  • I think I'm saying the same. – ChronoTrigger Oct 20 '15 at 00:56
  • "You have to call free(foo.b) by yourself before the foo variable in barFunc goes out of scope." - No, the pointer to the `malloc()`'d memory is returned to the caller because the structure is returned. The value of the pointer is copied to the caller's `struct Foo` and can be `free()`'d by the caller. It's exactly the same as passing just a pointer - the copy of the pointer goes out of scope in the allocating function, too, but it's value is returned. – Andrew Henle Oct 20 '15 at 01:02
  • To add my 2¢: "out of scope" is probably just sloppy wording here. It goes "out of scope" as soon as the function creating the struct ends, but a copy was passed to the caller. What you mean is (and that's somewhat clear by your wording "in `barFunc`") before it gets lost. –  Oct 20 '15 at 01:08
  • Trying to make it more clear: it wouldn't mind going "out of scope" in `barFunc` either as long as it would be passed somewhere else. Disregarding this really minor wording inaccuracy, I'd say this is a correct answer. –  Oct 20 '15 at 01:10
  • Ok, I think there is a misunderstanding here. Variable `foo` in `makeFoo` is returned and a copy of the pointer to the allocated memory is obtained. Then, the only pointer to the memory is kept in the `foo` variable in `barFunc`, which is the one I was talking about and the one that must be deallocated before the `barFunc` function finishes. Do we all agree? – ChronoTrigger Oct 20 '15 at 01:13
  • @ChronoTrigger I agree with you, and I think what Andrew was picking on is the fact that "going out of scope" there isn't the root problem ... the root problem is the last copy is going out of scope, so the pointer is effectively lost. I think it's really just a minor wording inaccuracy, a "nitpick" :) –  Oct 20 '15 at 01:16
  • I think we talked about different variables all the time. But -I'll be nitpicking now :) - the variable `foo` in `barFunc` going out of scope is actually the problem (in this exact case) because the pointer is not copied to any other place. Variable `foo` in `makeFoo` going out of scope is not a problem because its contents are copied on return. – ChronoTrigger Oct 20 '15 at 01:21
  • @ChronoTrigger exactly right. Mind to explain this in a half sentence in your answer (keyword: last copy)? I wouldn't have picked on it, but, as it came up, we're after perfect answers, right? :) –  Oct 20 '15 at 01:35
-1

Yes it Is it safe to return a struct from a C/C++ function, but struct with large static data as struct fooo{int a[1024*1024*1024];}; can cause program to crash. so it is much safer to return a pointer to an allocated block of memory for similar structs.

milevyo
  • 2,165
  • 1
  • 13
  • 18
  • That is not "the correct way". Returning a structure by value is perfectly legal and well-defined - the same as returning only a pointer by value is perfectly legal and well-defined. – Andrew Henle Oct 20 '15 at 00:48
  • Well, not my downvote, but ideed, returning a struct is perfectly safe (but potentially inefficient if the struct isn't reasonably small for doing such a thing) –  Oct 20 '15 at 00:54
  • should i delete my post, or just keep it – milevyo Oct 20 '15 at 00:55
  • @milevyo it's easy: `return` takes a value that is passed to the caller. In case of a struct, this could be quite some work (copying all the contents to the variable the caller declared). **If** this value happens to be a pointer and it points to some value on the local stack, **then** (and only then) you will have a problem, because this stack is gone when the function exits. –  Oct 20 '15 at 00:57
  • i understand now, function copy the content of the local variable in the stack to the LValue before it terminate. – milevyo Oct 20 '15 at 01:01
  • I agree with this answer. copying a struct, especially a large struct (large enough to not fit into a couple of registers) results in the compiler invoking memcpy(), usually two times when passing the struct in and one when returning the struct. These calls to memcpy() are hidden from the user, making them a problem. and the calls to memcpy() suck up CPU cycles that (especially in real-time embedded systems) cannot be casually tossed away. – user3629249 Oct 20 '15 at 01:15
  • @user3629249 the struct in this example is 8 bytes on your typical i386 architecture (or 16 bytes, due to 4 padding bytes, on amd64). That's not exactly a size where passing by value is a problem. I agree with this answer in that the code shown is good for many use cases. I disagree with it in that this is *not* the (only) "correct way". All in all, no vote at all from me. –  Oct 20 '15 at 01:20
  • @user3629249 I don't see any memcpy calls [in this example](http://goo.gl/eoVdV4). I left out the function body intentionally to prevent optimization. – M.M Oct 20 '15 at 01:20
  • @user3629249 and don't forget: heap management is expensive, too. –  Oct 20 '15 at 01:22
  • @user3629249 To give you an idea about that (normally "neatly" hidden behind a standard library function) ... for some toy project with MS-DOS, I just *had* to roll my own and it [looks like this](https://github.com/Zirias/clang-libdos/blob/master/src/stdlib/stdlib_malloc.c). This of course is extra strange for being as space efficient as I could manage (64KB segment!) but it really doesn't get MUCH better ... –  Oct 20 '15 at 01:29