0

I don't get a error message when I compile the code but I cant a proper result.

#include <iostream>
using namespace std;

struct Coord{
 int r;
 int c;
 };
struct CoordwValue{
 Coord C;
 char Value;
 };

CoordwValue* getNeighbors();

int main (){
 CoordwValue *k= getNeighbors();
 for (int i=0;i<4;i++)
  cout<<(k[i].Value);
}
CoordwValue *getNeighbors(){
 CoordwValue Neighbors[4];
 Neighbors->Value='X';
 Neighbors->C.r= 0;
 Neighbors->C.c= 1;
 (Neighbors+1)->Value='0';
 (Neighbors+1)->C.r= 1;
 (Neighbors+1)->C.c= 2;
 (Neighbors+2)->Value='1';
 (Neighbors+2)->C.r= 2;
 (Neighbors+2)->C.c= 1;
 (Neighbors+3)->Value='X';
 (Neighbors+3)->C.r= 1;
 (Neighbors+3)->C.c= 0;
 //for (int i=0;i<4;i++)
 // cout<<Neighbors[i].Value;
 return Neighbors;
 }

This part of the code prints X01X

for (int i=0;i<4;i++)
  cout<<Neighbors[i].Value;

But I can't get the same result from

for (int i=0;i<4;i++)
  cout<<(k[i].Value);

What is the problem?

Edit:

This version of the code works fine.

#include <iostream>
using namespace std;


char* getNeighbors();

int main (){
    char *k= getNeighbors();
    for (int i=0;i<4;i++)
        cout<<(*(k+i));
}
char *getNeighbors(Coord C, int r){
    char Neighbors[4];
    *Neighbors='X';
    *(Neighbors+1)='0';
    *(Neighbors+2)='1';
    *(Neighbors+3)='X'
    return Neighbors;
    }
James McNellis
  • 348,265
  • 75
  • 913
  • 977
redRagon
  • 3
  • 2

5 Answers5

4

You are returning a pointer to a stack-allocated array. This array will cease to exist when the function returns, so the pointer will effectively be invalid though it may still work (until you call another function, such as cout, when it will probably be wiped out by the new stack segment). You probably want to say this:

CoordwValue *Neighbors = new CoordwValue[4];

Instead of this:

CoordwValue Neighbors[4];

Of course, then it's up to the calling function (main in this case) to properly delete[] the array when it is finished using it.

cdhowie
  • 158,093
  • 24
  • 286
  • 300
  • @redDragon: This is called undefined behavior, effectively meaning your program's behavior is unpredictable – Armen Tsirunyan Nov 13 '10 at 21:48
  • This. And, as an additional hint, if you want to do ownership transfer, declare the return type of the `getNeighbours` function to be of type `std::auto_ptr` - safer than manually allocating the array and hoping the calle won't forget to free it, or properly implements exception handling. – Jim Brissom Nov 13 '10 at 21:50
  • 1
    @Jim: std::auto_ptr is an abomination and is (thank you very much) deprecated in C++0x. std::unique_ptr provides a superior alternative – Armen Tsirunyan Nov 13 '10 at 21:51
  • Is there a reason to call `delete` if you expect the program to finish execution just after `new` was called? The memory is cleaned up anyway after the application terminates. – Leonid Nov 13 '10 at 21:56
  • 1
    @Leonid: Yes, yes, yes, yes, yes. For one thing, `delete` doesn't just free memory, it destroys objects. If you have objects that manage non-memory resources (files, registry keys, streams, sockets, synchronization objects, etc.) then this is be very, very important. For another thing, it's _wrong_ not to clean up after yourself. – James McNellis Nov 13 '10 at 22:03
  • @Leonid A) if you get in the habit of not deleting when it doesn't matter, will you be as likely to remember when it does matter? B) what happens if the code is rearranged so it does matter, but the missing delete is hidden somewhere unnoticed C) what if the object's destructor performs some function (either now or in the future) that has an effect outside just the memory used by the application. – TheUndeadFish Nov 13 '10 at 22:05
  • Should I use delete[]k or delete[] Neighbors? I think delete[]k but I want to be sure :) – redRagon Nov 13 '10 at 22:06
  • 1
    @redRagon: You should do neither: use `std::vector` or a smart pointer. – James McNellis Nov 13 '10 at 22:08
  • This is a "just don't do it this way" situation. Return std::string or vector if you have to allocate and return in a function. In C++0x you will be able to return by r-value, i.e. "move" the memory so it doesn't have to make that copy, although with RVO will probably optimise it already. If you really want to be certain to save the copy then pass in a buffer as a parameter, ideally std::string& or std::vector – CashCow Nov 13 '10 at 22:15
3

If you want to return an array of four objects, you don't necessarily need to use dynamic allocation or std::vector. You just need to wrap the array in a class so that you can return it. For example:

struct GetNeighborsResult
{
    CoordwValue Value[4];
};

GetNeighborsResult getNeighbors();

Boost, TR1, and C++0x all have a container-like array class template that you can easily use for this purpose:

std::array<CoordwValue, 4> getNeighbors();

The advantage of using array is that you don't have to write a separate class for every type and number that you have, you can just use the class template.

If you do choose to return a pointer to a dynamically allocated array, use a smart pointer to manage the memory. There is no reason whatsoever to not use a smart pointer.

James McNellis
  • 348,265
  • 75
  • 913
  • 977
1

Returned array is created on the stack and returned. I'd suggest to read about difference of heap and stack memory here.

If array needs to be returned from a function you have an option of allocating memory dynamically using new. However, then the memory must be released with delete or it will result in a memory leak.

STL containers use dynamic memory and have overloaded copy constructor. Replacing array with vector<T> will allow you to return the values safely.

Community
  • 1
  • 1
Leonid
  • 22,360
  • 25
  • 67
  • 91
0

The problem is that you are returning a variable on the stack. The variable Neighbors is created in on the stack in the method getNeighbors. When you leave this method, the memory is destroyed, corrupting your return value.

How to fix it? Pass in an array created on the outside and fill the values in.

Starkey
  • 9,673
  • 6
  • 31
  • 51
0

You are returning the address of a local variable. When getNeighbors returns, Neighbors[4] goes out of scope, causing all sorts of problems, including what should be a compiler warning/error.

You have a couple of options around this: first, do what cdhowie said and return a dynamically allocated array. Another is to return by value, so the return is a COPY of Neighbors[4], not a pointer to it. I think the syntax for this would be something like CoordwValue getNeighbors()[4] { .... }

Yet another is to have the caller pass in a pre-allocated array that you fill in.

tenfour
  • 36,141
  • 15
  • 83
  • 142
  • The syntax you use is correct, but the semantics you want to apply is not: A function cannot have a return type of array type (no array and no function type). – Johannes Schaub - litb Nov 13 '10 at 21:58