0

I'm currently sitting with an assignment in which you are supposed to read spheres from a file. A typhical input from the file can look like this: "0.182361 -0.017057 0.129582 0.001816". Representing the, x, y and z coordinates plus the Spheres radius. While reading the File I'm using my own method: "AddSphere" which adds a sphere into an array.

void Array::AddSphere(MySphere inp)
{
if (length == 10000 || length == 200000 || length == 30000)
{
    ResizeBuffer(); 
}
this->length++;
*arr = inp;
arr++;
//this->length++;
}

The class "Array" is supposed to be like a holder for all spheres and is containing the variable "arr" that is pointing on the current element. 

    class Array
    {
    public :

    int length;
    void AddSphere(MySphere inp);
    int arrLength();
    void RemoveAt(int pos);
    void AddFromFile();
    MySphere * Buffer;
    void CreatenewBuffer();
    private:
    MySphere * arr;

    public:Array()
       {
           Buffer = new MySphere[10000];
           arr = Buffer;
           length = 0;
       }
       ~Array()
       {
           delete[] arr;
           delete[] Buffer;
       }
};

It is also containing "Buffer" that is pointing on the first element in "arr". So now what is my problem? The problem is that i want to be able to dynamicly increase the Buffer's size whenever "length" equals a specified value. Lets say my file contains 15000 elements. Then i need to increase the Buffers size to be able to have the correct length on the array.with ResizeBuffer() i try to do that.

    void Array::ResizeBuffer()
{

    MySphere * tempBuff = new MySphere[length+10000];
    memcpy(tempBuff, Buffer, sizeof((MySphere)*Buffer));
    this->Buffer = new MySphere[length+10000];
    arr = Buffer;
    memcpy(Buffer, tempBuff, sizeof((MySphere)*tempBuff));


    //int a = 0;

    }

But for some reason i only get the last 5000 elements in my output instead of all the 15000. I figured that it has something to do with the pointer of arr not being pointed to the whole buffer, but none of my attempts work. So why is this happening?

Thank you for your time!

Improve this question
9
  • 3
    Any reason you're not using std::vector? Commented Jan 28, 2013 at 16:35
  • 3
    Yes, i'm supposed to create my own form of holderclass, implemented from without libraries Commented Jan 28, 2013 at 16:36
  • Also need the code to be as fast as possible Commented Jan 28, 2013 at 16:39
  • Wow, ResizeBuffer has got to be one of the more confused functions I've seen. Commented Jan 28, 2013 at 16:39
  • 4
    Few Points: 1) Strongly prefer std::copy over memcpy, since it will always do the right thing instead of failing miserably for classes with user defined copy operations. 2) Why do you use the size of a pointer as the length argument of memcpy? 3)Why store the pointer twice (arr and Buffer) Why do you copy your data around twice? 4) You are leaking memory, since you never delete your data. 5) Why do you copy the data twice, instead of just setting Buffer to tempBuff? Commented Jan 28, 2013 at 16:43

4 Answers 4

Reset to default
3

Couple of problems:

The dynamic data is only allocated once (though you have two pointers to the arrea).

Array()
   {
       Buffer = new MySphere[10000];
       arr = Buffer;
       length = 0;
   }

SO you can only delete it once (one allocation one destruction).

   ~Array()
   {
       delete[] arr;
       delete[] Buffer;  // Double delete.
   }

When copying objects you can not use memcpy (unless the object falls under a very special subcategory). If you are going to use C++ with methods and all its other features this is unlikely to be the case. Prefer to use std::copy() to copy the content from one array to another.

MySphere * tempBuff = new MySphere[length+10000];
memcpy(tempBuff, Buffer, sizeof((MySphere)*Buffer));
this->Buffer = new MySphere[length+10000];
arr = Buffer;
memcpy(Buffer, tempBuff, sizeof((MySphere)*tempBuff));

Also why are you copying it twice?

Note in addition to using std::copy
The resize should happen in three distinct phases. 

1. Allocate and copy data       // Dangerous may throw
2. Reset object                 // Safe
3. Release old resources.       // Dangerous may throw

So this is how it should look.

// Phase 1: Allocate and copy.
std::unique_ptr<MySphere> tempBuffer = new MySphere[length+1000];   // Hold in a smart pointer
                                                                    // For exception safety
// Prefer to use std::copy. It will use the objects copy constructor
std::copy(Buffer, Buffer+length, MySphere);                         // Copies the objects correctly. 


// Everything worked so Phase 2. Reset the state of the object.
// We can now put the tempBuffer into the object
MySphere* toDelete = Buffer;                                        // keep old buffer for stage 3.
Buffer = tempBuffer.release();
length += 1000;

// Phase 3
// State of the new object is consistent.
// We can now delete the old array
delete [] toDelete;

If you are not allowed to use a smart pointer. Then replace phase 1 with this:

// Phase 1: Allocate and copy.
MySphere* tempBuffer = new MySphere[length+1000];

try                                                 // For exception safety
{
    // Prefer to use std::copy. It will use the objects copy constructor
    std::copy(Buffer, Buffer+length, MySphere);     // Copies the objects correctly. 
}
catch(...)
{
    delete [] tempBuffer;
    throw;
}

Since your class has taken ownership of a dynamic allocated object. Your class should also implement the rule of three. This means you need to define the copy constructor and the assignment operator.

What you really need to have dynamic array are three things:

    1) A pointer to the buffer.
    2) The current size the user knows about (reported by size)
    3) The actual size. At which point you need to resize

Here is an example of how to implement the rule of three for an array:

https://stackoverflow.com/a/255744/14065

Anything else is superfluous.

Unnecessary in your case as this is a project. But for extra marks look up how to use placement new. This should prevent extra initialization of objects that don't really exist in your array.

An example of how to use place new:

https://stackoverflow.com/a/13994853/14065

Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

I think i get the idea behind it! The only thing that confuses my abit is the Buffer = tempBuffer.release(); - what does it do?
@JakobDanielsson. It releases the newly allocated pointer from the smart pointer. I put it in a smart pointer in case the copy generated an exception. If an exception was generated then the smart pointer guarantees that the memory will be cleaned up correctly.
I have updated the code in case you can't use a smart pointer. If you use this alternative you do not need release().
Thank you! For some reason i can't use MySphere as an argument for copy, but maybe that's a question for another topic! Thanks once again
@JakobDanielsson: MySphere is a type. You want to pass the name of the pointer.
|
1

You aren't updating your length variable within your Array::CreatenewBuffer() function.

I'm assuming where you are iterating through the objects you probably have something like this...

for (size_t i = 0; i < Array.length; ++i)
    printf("Item found");

The way you are using length is actually fairly confusing now that I look back. You may want to separate it into two variables: size and capacity. Capacity would be the amount of objects that the currently allocated buffer can hold and size is the amount of objects that have actually been added to the buffer.

Improve this answer

1 Comment

length represents size, but having a max length is definatly a good idea! I'll steal it from you! Thank you
1

How about this?

void Array::CreatenewBuffer()
{
    MySphere * tempBuff = new MySphere[length+10000];
    std::copy ( Buffer, Buffer+length, tempBuff );
    delete[]Buffer;
    Buffer = tempBuff;
    delete[]tempBuff;
}
Improve this answer

1 Comment

You really think this is a good idea: Buffer = tempBuff; delete[]tempBuff;?
0

Are the values for each sphere entered on separate lines? If so, you could simply count the lines and then initialise the buffer with the correct value.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.