«

Hi,

do you think the following code could lead to memory leaks? it creates a
circualr buffer made up of 5 element then it pushes 10 elements (overwriting
the first 5) when overwriting , do you think the eldest pointed memory will
be overwritten, or a new pointer in memory will be written?

#define _CRT_SECURE_NO_WARNINGS
#include <windows.h>
#include <vector>
#include <cstdlib>
#include <ctime>
#include <cstdio>
#include <boost/circular_buffer.hpp>
using namespace std;
using namespace boost;

char * getDateTime(void);
const short numbuff = 5;
const short buflen = 30;

typedef struct
{
unsigned char * pData;
unsigned short bufferLength;
unsigned short bytesRecorded;
bool flag;
} Buffer;

int main()
{
circular_buffer<Buffer*> cb(numbuff);
circular_buffer<Buffer*>::const_iterator it;

printf("Push elements:\n");
// fill buffer
for(int i = 0; i<10; i++)
{
// set up buffer
Buffer *buff = new Buffer;
ZeroMemory(buff, sizeof(Buffer));

buff->bufferLength = buflen;
buff->bytesRecorded = buflen;
buff->flag = true;
buff->pData = new unsigned char[buflen];
buff->pData = reinterpret_cast<unsigned char *>(getDateTime());

printf("%s\n", buff->pData);

// push buffer
cb.push_back(buff);

Sleep(1000);
}

printf("\nShow elements:\n");

// show elements
for(int i = 0; i<static_cast<int>(cb.size()); i++)
{
printf("%s\n", cb[i]->pData);
}

system("pause");
return EXIT_SUCCESS;
}

// getDateTime (Fri, 10 Oct 2008 14:41:59 GMT)
char * getDateTime(void)
{
time_t rawtime;
struct tm * timeinfo;
time(&rawtime);
timeinfo = gmtime(&rawtime);
char * buffer = new char[30];
strftime(buffer,30,"%a, %d %b %Y %X GMT",timeinfo);
return buffer;
}

// thanks

* Larry:
> Hi,
>
> do you think the following code could lead to memory leaks? it creates
> a circualr buffer made up of 5 element then it pushes 10 elements
> (overwriting the first 5) when overwriting , do you think the eldest
> pointed memory will be overwritten, or a new pointer in memory will be
> written?

The question is unclear, but yes, you have memory leaks.

The memory leaks are due to coding at the C level.

In C++ use library classes, such as replacing your Buffer with std::string.



> #define _CRT_SECURE_NO_WARNINGS

Don't do that.


> #include <windows.h>
> #include <vector>
> #include <cstdlib>
> #include <ctime>
> #include <cstdio>
> #include <boost/circular_buffer.hpp>
> using namespace std;
> using namespace boost;
>
> char * getDateTime(void);

Let that function return std::string.

By the way, 'void' as argument type is a C-ism.

It doesn't make much sense in C++.


> const short numbuff = 5;
> const short buflen = 30;

This confuses two different kinds of buffers: your circular buffer with room for
5 strings, and your string type named "Buffer" with room for 30 characters.

You don't need the latter constant.

For the first constant, consider replacing 'short' with 'int': there's no good
reason to choose anything but 'int' here.


> typedef struct
> {
> unsigned char * pData;
> unsigned short bufferLength;
> unsigned short bytesRecorded;
> bool flag;
> } Buffer;

Just remove that.


> int main()
> {
> circular_buffer<Buffer*> cb(numbuff);

This would be

circular_buffer<string> timeStrings( bufferSize );


> circular_buffer<Buffer*>::const_iterator it;
>
> printf("Push elements:\n");
> // fill buffer
> for(int i = 0; i<10; i++)
> {
> // set up buffer
> Buffer *buff = new Buffer;
> ZeroMemory(buff, sizeof(Buffer));
>
> buff->bufferLength = buflen;
> buff->bytesRecorded = buflen;
> buff->flag = true;
> buff->pData = new unsigned char[buflen];
> buff->pData = reinterpret_cast<unsigned char *>(getDateTime());

Don't cast.

>
> printf("%s\n", buff->pData);
>
> // push buffer
> cb.push_back(buff);

This would be simply

timeStrings.push_back( getDateTime() );


> Sleep(1000);
> }
>
> printf("\nShow elements:\n");
>
> // show elements
> for(int i = 0; i<static_cast<int>(cb.size()); i++)
> {
> printf("%s\n", cb[i]->pData);
> }
>
> system("pause");
> return EXIT_SUCCESS;
> }
>
> // getDateTime (Fri, 10 Oct 2008 14:41:59 GMT)
> char * getDateTime(void)
> {
> time_t rawtime;
> struct tm * timeinfo;
> time(&rawtime);
> timeinfo = gmtime(&rawtime);
> char * buffer = new char[30];
> strftime(buffer,30,"%a, %d %b %Y %X GMT",timeinfo);
> return buffer;
> }

See above.


Cheers & hth.,

- Alf

On 25 Jan., 17:01, "Larry" <dontmewit...@got.it> wrote:
> Hi,
> * *do you think the following code could lead to memory leaks?

Yes. In this case it's fairly easy to determine: You used the "new"-
operator three times but never the "delete"-operator and never passed
on the responsibility for managing the object life times to other
functions or objects.

> typedef struct
> {
> *unsigned char * pData;
> *unsigned short bufferLength;
> *unsigned short bytesRecorded;
> *bool flag;
> } Buffer;

Note: The type "Buffer" isn't responsible for managing the allocated
buffer. Otherwise you would have made the members private and added
allocation/deallocation/copy ctor/assignment operators.

> int main()
> {
> *circular_buffer<Buffer*> cb(numbuff);
> *circular_buffer<Buffer*>::const_iterator it;
>
> *printf("Push elements:\n");
> *// fill buffer
> *for(int i = 0; i<10; i++)
> *{
> * // set up buffer
> * Buffer *buff * * * = new Buffer;

These Buffer objects are never deleted.

> * ZeroMemory(buff, sizeof(Buffer));
>
> * buff->bufferLength *= buflen;
> * buff->bytesRecorded = buflen;
> * buff->flag * * * * *= true;
> * buff->pData * * * * = new unsigned char[buflen];

These chunks of memory buff->pData points to are never deleted.

> // push buffer
> cb.push_back(buff);

The comment is wrong. You don't push a buffer. You push a pointer to a
struct that contains a pointer to the "buffer". That's the problem.
That's your leak. The container simply stores these pointers and is
not responsible for deleting your stuff. Containers generally don't
care about what kind of objects they store and they don't treat
pointers specially. Note: "object" and "pointer" are not mutually
exclusive. You can have an object that IS a pointer and containers a
la vector<int*> stores pointer objects. If you destroy a pointer,
nothing special happens -- specifically, the pointed to object is not
touched or free'd automatically.

> // getDateTime (Fri, 10 Oct 2008 14:41:59 GMT)
> char * getDateTime(void)
> {
> time_t rawtime;
> struct tm * timeinfo;
> time(&rawtime);
> timeinfo = gmtime(&rawtime);
> char * buffer = new char[30];
> strftime(buffer,30,"%a, %d %b %Y %X GMT",timeinfo);
> return buffer;
> }

Yet another new[] for which I don't see a delete[].

The rule is simple: If you new'd something you should delete it again
-- expect when you pass the pointer to some function or object that
_specifically_ takes ownership (responsibility for managing the life-
time) of the POINTEE (the thing the pointer points to).

Cheers,
SG

"Alf P. Steinbach" <> ha scritto nel messaggio
news:hjkg1c$88b$...

> The question is unclear, but yes, you have memory leaks.
>
> The memory leaks are due to coding at the C level.
>
> In C++ use library classes, such as replacing your Buffer with
> std::string.

That's great of you! Yet, I might be dealing with some data returned to me
as <unsigned char>. If it is possible to cast from unsigned char to string I
will be totally follow your advice. (I am dwaling with binary data mostly!)

thanks

* Larry:
> "Alf P. Steinbach" <> ha scritto nel messaggio
> news:hjkg1c$88b$...
>
>> The question is unclear, but yes, you have memory leaks.
>>
>> The memory leaks are due to coding at the C level.
>>
>> In C++ use library classes, such as replacing your Buffer with
>> std::string.
>
> That's great of you! Yet, I might be dealing with some data returned to
> me as <unsigned char>. If it is possible to cast from unsigned char to
> string I will be totally follow your advice. (I am dwaling with binary
> data mostly!)

In practice you'll have to cast the pointer then, because
std::basic_string<unsigned char> is somewhere in the gray zone of not quite well
specified.

But other than that, the std::string constructors let you copy the data as
zero-terminated or as n bytes, whatever you have.

And std::string deals well with zero bytes, no problems.

However, if this is just raw data, consider

typedef unsigned char Byte;
typedef vector<Byte> ByteVector;

Cheers & hth.,

- Alf

Larry wrote:
> "Alf P. Steinbach" <> ha scritto nel messaggio
> news:hjkg1c$88b$...
>
>> The question is unclear, but yes, you have memory leaks.
>>
>> The memory leaks are due to coding at the C level.
>>
>> In C++ use library classes, such as replacing your Buffer with
>> std::string.
>
> That's great of you! Yet, I might be dealing with some data
> returned to me as <unsigned char>. If it is possible to cast from
> unsigned char to string I will be totally follow your advice. (I am
> dwaling with binary data mostly!)
> thanks

So, if it is not a string you could try std::vector<unsigned char>
instead. That's a good type for a byte buffer.


Bo Persson

"Bo Persson" <> ha scritto nel messaggio
news:...

> So, if it is not a string you could try std::vector<unsigned char>
> instead. That's a good type for a byte buffer.

That's what I have been looking for!

Anyway now I have trouble with the following:

void getDateTime(char * szTime);
const int numbuff = 5;
const int buflen = 30;

struct Buffer
{
public:
vector<char> vChar;
unsigned int bufferLength;
unsigned int bytesRecorded;
Buffer() : bytesRecorded(0), bufferLength(0), vChar(NULL) { };
};

int main()
{
circular_buffer<Buffer> cb(numbuff);
circular_buffer<Buffer>::const_iterator it;

for(int i = 0; i<10; i++)
{
// Get time
char szTime[30]; getDateTime(szTime);

// Init Buff
Buffer buff;
ZeroMemory(&buff, sizeof(Buffer));

buff.vChar.resize(buflen);
buff.vChar = szTime;
buff.bufferLength = buflen;
buff.bytesRecorded = buflen;

printf("%s\n", buff.vChar);
}

system("pause");
return EXIT_SUCCESS;
}

// getDateTime (Fri, 10 Oct 2008 14:41:59 GMT)
void getDateTime(char * szTime)
{
time_t rawtime = time(NULL);
struct tm timeinfo;
gmtime_s(&timeinfo, &rawtime);
strftime(szTime, 30, "%a, %d %b %Y %X GMT", &timeinfo);
}

The code fails here: buff.vChar = szTime;

???

thanks

"Alf P. Steinbach" <> ha scritto nel messaggio
news:hjki9g$rc0$...
>* Larry:
>> "Alf P. Steinbach" <> ha scritto nel messaggio

> However, if this is just raw data, consider
>
> typedef unsigned char Byte;
> typedef vector<Byte> ByteVector;

the new struct should look like this:

struct Buffer
{
public:
vector<unsigned char> vuChar;
int bufferLength;
int bytesRecorded;
int user;
Buffer() : bytesRecorded(0), bufferLength(0), vChar(NULL), user(0) { };
};

what the difference between:
--> vector<unsigned char> vuChar;
and
--> unsigne char buffer[]

??

thanks

In message <4b5dcf0e$0$819$>, Larry
<> writes
>
>"Bo Persson" <> ha scritto nel messaggio
>news:...

[aside: why is everyone posting low-level C code this week?]

>
>> So, if it is not a string you could try std::vector<unsigned char>
>>instead. That's a good type for a byte buffer.
>
>That's what I have been looking for!

You're using strftime and printf("%s") on it. That looks like a string
to me.
>
>Anyway now I have trouble with the following:
>
>void getDateTime(char * szTime);

Can't you make this into a function that returns a std::string?

>const int numbuff = 5;
>const int buflen = 30;
>
>struct Buffer
>{
>public:
>vector<char> vChar;
>unsigned int bufferLength;
>unsigned int bytesRecorded;

What's the point of bufferLength and bytesRecorded now? vector does its
own housekeeping, so there's no need for you to.

>Buffer() : bytesRecorded(0), bufferLength(0), vChar(NULL) { };

Lose that. vector's default constructor will work fine.

>};
>
>int main()
>{
>circular_buffer<Buffer> cb(numbuff);
>circular_buffer<Buffer>::const_iterator it;

You declare these but never use them.

>
>for(int i = 0; i<10; i++)
>{

And you do this 10 times for no good reason. I think you've lost track
of the need for a circular buffer somewhere along the way ;-(

> // Get time
> char szTime[30];

Is that 30 the same as the "buflen" you declared earlier?
Why the Hungarian prefix?

>getDateTime(szTime);
>
> // Init Buff
> Buffer buff;
> ZeroMemory(&buff, sizeof(Buffer));

Don't do that. Buffer's constructor should do all you need to do to
initialise it.
>
> buff.vChar.resize(buflen);

Don't do that. Assigning to the buffer will do all that's necessary.

> buff.vChar = szTime;

buff.vChar.assign(szTime, szTime+strlen(szTime));

Better still, give Buffer a constructor with appropriate arguments which
initialises the buffer.

> buff.bufferLength = buflen;
> buff.bytesRecorded = buflen;

Redundant. Just use buff.vChar.size();

>
> printf("%s\n", buff.vChar);

printf("%s\n", &buff.vChar[0]);

>}
>
>system("pause");
>return EXIT_SUCCESS;
>}
>
>// getDateTime (Fri, 10 Oct 2008 14:41:59 GMT)
>void getDateTime(char * szTime)
>{
>time_t rawtime = time(NULL);
>struct tm timeinfo;
>gmtime_s(&timeinfo, &rawtime);
>strftime(szTime, 30, "%a, %d %b %Y %X GMT", &timeinfo);

Is that "30" the same as the "buflen" you declared earlier?

>}
>
>The code fails here: buff.vChar = szTime;

If you'd used string instead of vector<char> it would have worked
Because it's such a common operation, string (unlike vector) has an
assignment operator that takes a pointer to a C-style null-terminated
string.
>
>???

Try "Accelerated C++".

>
>thanks
>

--
Richard Herring

"Richard Herring" <junk@[127.0.0.1]> ha scritto nel messaggio
news:...

> And you do this 10 times for no good reason. I think you've lost track of
> the need for a circular buffer somewhere along the way ;-(

I am just tring the circular buffer overwritten.

anyway the following code does not work:

#include <windows.h>
#include <vector>
#include <cstdlib>
#include <ctime>
#include <cstdio>
#include <boost/circular_buffer.hpp>
using namespace std;
using namespace boost;

void getDateTime(char * szTime);
const int numbuff = 5;
const int buflen = 30;

struct Buffer
{
public:
vector<char> vChar;
int bufferLength;
int bytesRecorded;
int user;
Buffer() : bytesRecorded(0), bufferLength(0), user(0) { };
};

int main()
{
circular_buffer<Buffer> cb(numbuff);
circular_buffer<Buffer>::const_iterator it;

// Insert elements
for(int i = 0; i<10; i++)
{
// Get time
char szTime[30]; getDateTime(szTime);

// Init Buff
Buffer buff;
copy(&szTime[0],&szTime[30],std::back_inserter(buff.vChar));

//buff.vChar.assign(szTime, szTime+strlen(szTime));
buff.bufferLength = buflen;
buff.bytesRecorded = buflen;
buff.user = i;

printf("%d, %d, %s\n", buff.user, buff.bufferLength, szTime);

cb.push_back(buff);
}

// Show elements:
for(int i = 0; i<(int)cb.size(); i++)
{
printf("%d, %d, %s\n", cb[i].user, cb[i].bufferLength, cb[i].vChar);
}

system("pause");
return EXIT_SUCCESS;
}

// getDateTime (Fri, 10 Oct 2008 14:41:59 GMT)
void getDateTime(char * szTime)
{
time_t rawtime = time(NULL);
struct tm timeinfo;
gmtime_s(&timeinfo, &rawtime);
strftime(szTime, 30, "%a, %d %b %Y %X GMT", &timeinfo);
}

in the second loop I get <null> when I try to print vChar

NB: in the near future I may be deal with unsigned char data so that's why I
cannot have getDateTime() return std::string