Why aren't objects being pushed to this vector (when it's a member of a struct)?

Question

I have the following code, which loops through an array of menuoptions and on each iteration, creates a ScaledRect object and pushes it to a vector. This vector is a member of a struct.

I have verified that the ScaledRect is created with the correct values, yet when I print back the contents of the regions vector ( in the second loop ), the loop never terminates and the values are garbage.

class ScaledRect : public Rect
{
public:
    ScaledRect(int x1, int y1, int x2, int y2);
};
ScaledRect::ScaledRect(int x1, int y1, int x2, int y2):
    _x1(x1), _y1(y1), _x2(x2), _y2(y2){}

// ScaledRect doesn't have copy constructor, but Rect does
Rect::Rect( const Rect &rect)
{
x1=rect.x1; y1=rect.y1; x2=rect.x2; y2=rect.y2; bClean=KD_FALSE;
}

typedef struct
{
    std::vector<ScaledRect> regions;
}interface;

void PushRegions( interface * myself )
{
    int i = 0;
    while(menuoptions[i].callback != -1 )
    {
        ScaledRect s = 
          ScaledRect(menuoptions[i].x1, 
                     menuoptions[i].y1,
                     menuoptions[i].x2, 
                     menuoptions[i].y2);            
        myself->regions.push_back( s );
        i++;
    }

    std::vector<ScaledRect>::iterator iter = myself->regions.begin();
    std::vector<ScaledRect>::iterator done = myself->regions.end();

    while(iter != done)
    {
        iter->Dump();
        iter++;
    }
}

EDIT Please note - I've just edited - the memory for theinterface is created and I do actually pass in the address of theinterface to this function. (However, I have simplified those two lines here - what actually happens is that PushRegions gets called via a ptr to a function, on a piece of newly allocated memory the size of an interface ).

I can't post all of the code here - but minimally its:

Func pfunc = GetPFuncForInterfaceObj();
size_t  numbytes = GetSizeForInterfaceObj();
char memory = new char[numbytes];
pfunc(memory);

pfunc ends up being PushRegions and memory ends up being passed as an interface.

When I push the ScaledRect object to a vector declared at the top of PushRegions() it works. Has anyone got any ideas why?

Answer 1

Hard to say without seeing the rest of ScaledRect - does it have a working copy constructor and assignment operator? What does Dump do?

Especailly - why is i uninitialized? Is this (all) the (current) code?

Answer 2

You don't seem to be initializing i in the PushRegions() function. Are you sure it starts at 0?

Follow-up to your edits:

what actually happens is that PushRegions gets called via a ptr to a function, on a piece of newly allocated memory the size of an interface

What does this mean? Are you actually invoking the interface constructor?

Follow up to your comment:

Yes, there is a (compiler-generated) constructor. The std::vector data member has a default constructor and is invoked by the compiler-generated constructor for interface. When you allocate the right amount of memory, without invoking the constructor, you get an uninitialized std::vector!

This explains why your loop never terminates and garbage results: you're using an uninitialized object, which results in undefined behavior.

Answer 3

This is utterly wrong:

size_t  numbytes = GetSizeForInterfaceObj();
char memory = new char[numbytes];
pfunc(memory);

Even if we "fix" it:

size_t  numbytes = GetSizeForInterfaceObj();
char* memory = new char[numbytes]; // note pointer
pfunc((interface*)memory); // and cast

Your object has never been constructed, so the vector is in a garbage state. (Using the object leads to undefined behavior.)

No, interface may not have a constructor explicitly defined, but there is an implicit constructor, and it's there for a reason. It needs to construct the members. You can use "placement new" (by including <new>) to construct an object by placing it at a memory location:

size_t  numbytes = GetSizeForInterfaceObj();
char* memory = new char[numbytes]; // note pointer
pfunc(new (memory) interface); // and CREATE

Now you're using a valid object.

---

I'll assume there's a good reason for using pointers at all, let alone a manually constructed object. That said, your code does too much. It both manages a resource, and uses one; pick one or the other.

That is:

struct interface_obj
{
    interface_obj() :
    mMemory(GetSizeForInterfaceObj()),
    mInterface(new (&mMemory[0]) interface)
    {}

    ~interface_obj()
    {
        mInterface->~interface(); // destruct
    }

    interface* get() const
    {
        return mInterface;
    }

private:
    // noncopyable for now, easy to add
    interface_obj(const interface_obj&);    
    interface_obj& operator=(const interface_obj&);

    // again, with the vector we use a resource (dynamic buffer),
    // not manage one.
    std::vector<char> mMemory;
    interface* mInterface;
};

Much cleaner:

interface_obj obj;
pfunc(obj.get());

And it will be released no matter what. (Your code wouldn't in the face of exceptions, without messy try-catch blocks and other nonsense.) Again, preferable is to not have this kind of allocation in the first place.