1

I'm using Visual c++.

I'm trying to implement a circular buffer, this CB must handle a specific type of data...in fact, it's a structure data where we have some kind of raw data to be stored in a char type and a date associated to that data...this has been implemented using a strucuture.

here is the code for more details:

#include <stdio.h> #include <time.h> #include <windows.h> //data=date_label+raw_data typedef struct DataFragment { char data[4]; clock_t date; }DataFragment; typedef struct CircularBuffer { DataFragment *buffer; // data buffer DataFragment *buffer_end; // end of data buffer size_t capacity; // maximum number of items in the buffer size_t count; // number of items in the buffer size_t sz; // size of each item in the buffer DataFragment *head; // pointer to head DataFragment *tail; // pointer to tail } CircularBuffer; void cb_init(struct CircularBuffer *cb, size_t capacity, size_t sz) { if((cb->buffer = (DataFragment*) malloc(capacity * sz))!=NULL) puts("success alocation"); //if(cb->buffer == NULL) //handle error cb->buffer_end = (DataFragment *)cb->buffer + (capacity-1)*sz; cb->capacity = capacity; cb->count = 0; cb->sz = sz; cb->head = cb->buffer; cb->tail = cb->buffer; } void cb_free(struct CircularBuffer *cb) { free(cb->buffer); // clear out other fields too, just to be safe } void cb_push_back(struct CircularBuffer *cb, const DataFragment *item) { //if(cb->count == cb->capacity) //handle error when it's full memcpy(cb->head->data, item->data,4); cb->head->date=item->date; cb->head = (DataFragment*)cb->head + cb->sz; if(cb->head == cb->buffer_end) cb->head = cb->buffer; cb->count++; } void cb_pop_front(struct CircularBuffer *cb, DataFragment *item) { //if(cb->count == 0) //handle error memcpy(item->data, cb->tail->data,4); item->date=cb->tail->date; cb->tail = (DataFragment*)cb->tail + cb->sz; if(cb->tail == cb->buffer_end) cb->tail = cb->buffer; cb->count--; } int main(int argc, char *argv[]) { struct CircularBuffer pbuf; pbuf.buffer=NULL; pbuf.buffer_end=NULL; pbuf.capacity=0; pbuf.count=0; pbuf.head=NULL; pbuf.sz=0; pbuf.tail=NULL; struct CircularBuffer *buf= &pbuf; size_t sizz = sizeof(DataFragment); //initialisation of the circlar buffer to a total bytes //of capacity*sizz=100*sizeof(struct DataFragment) cb_init(buf,100,sizz); //temporary container of data DataFragment temp,temp2; for(int i=0;i<4;i++) temp.data[i]='k'; for(int i=0;i<4;i++) temp2.data[i]='o'; //pushing temporary buffer to the CB...40*2=80<capacity of the CB for(int i=0;i<40;i++) { Sleep(20); temp.date=clock(); cb_push_back(buf,&temp); Sleep(10); temp2.date=clock(); cb_push_back(buf,&temp2); } DataFragment temp3; for(int i=0;i<20;i++) { cb_pop_front(buf,&temp3); printf("%d\n", temp3.data); //print integers....no need of end caracter } cb_free(buf); return 0; }

When I compile the code, everything is fine, but when I debug, I noticed a problem with the buffer_end pointer, it says bad_pointer....this happens if the capacity is greater than 56...I don't know why the pointer can't point to end of the buffer.But if the capacity is less than 56 the pointer points exactly on the end of the buffer

If anyone knows why this happens like this, and how to fix it, please help me..

thanks in advance

Improve this question
4
  • Apart from <iostream>, this is pure C(99) code. Commented Feb 21, 2011 at 12:40
  • C with the worst feature of C++ :) Commented Feb 21, 2011 at 13:05
  • There, got rid of cout and now pure C Commented Feb 21, 2011 at 13:10
  • yes....just forgot cout there....I'm coding in C, but When it works, I intend to convert it to C++ Commented Feb 21, 2011 at 13:25

4 Answers 4

Reset to default
2

It seems you are misunderstanding pointer arithmetic

cb->buffer_end = (DataFragment *)cb->buffer + (capacity-1)*sz; cb->head = (DataFragment*)cb->head + cb->sz; cb->tail = (DataFragment*)cb->tail + cb->sz;

Pointer arithmetic already takes into account the size of the underlying type. All you really need is

++cb->head; ++cb->tail;

If the idea is to hack around sizeof(DataFragment) - perhaps to allocate more storage for one item than the struct's size - for some evil purpose - you'll need to first cast the pointer to a char* (because sizeof(char) == 1).

cb->tail = (DataFragment*)((char*)cb->tail + cb->sz);

Design-wise the struct appears to have too many members: buffer_end and capacity duplicate each other (given one you can always find the other), and the sz member is not necessary (it should always be sizeof(DataFragment).

Also, I believe you can just assign structs

*(cb->head) = *item;

there seem to be completely unnecessary casts (probably resulting from the misunderstanding of pointer arithmetic):

cb->buffer_end = (DataFragment *)cb->buffer + (capacity-1)*sz;

And if it is supposed to be C++, then it contains lots of "C-isms" (typedeffing structs, using struct XXX var; - despite having it typedeffed, etc), and the code is generally designed in a purely C style (not taking advantage of C++'s greatest strength, automatic resource management with RAII).

May I also point out that clock() hardly gives you a date :)

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

3 Comments

Yes I did misunderstood the pointer arithmetic... But, I can't just assign structs like *(cb->head) = *item I need to copy data not addresses from a temporary buffer to the circular buffer, so I have to use memcpy..
@fsidiosidi: Assigning the address would look like this: cb->head = item;. What I have shown dereferences the pointers, so you'll be assigning the struct objects themselves.
mmm...ok, but don't you think a binary copy does the job...I don't want to bother myself with casting type...also, the data i'm putting in the item is a raw data comming from the network...
2

I think you need to remove the * sz. (And I don't think you need the cast.)

cb->buffer_end = cb->buffer + (capacity-1);

Arithmetic on pointers automatically accounts for the size of the type pointed to.

I should also point out boost::circular_buffer.

Improve this answer

2 Comments

Ok, I see...I think I have to revise my pointers arithmetic....I don't know if Boost::circlar_buffer will stisfy my purpose....The idea behind this code is to allocate memory just one time in the beginning of the program....I can't use dynamic allocation...do you think boost library will satisfy this purpose?
@fsidiosidi, yes, boost::circular_buffer will allocate only once when constructed, unless you call its resize() function. (See third paragraph, here)
1

you are assuming that pointers are 4 byte wide. This may not be the case on all platforms (x86_64). Hence, the memcpy()'s should make use of the sizeof operator. There seems to be another bug with "end = buffer + (capacity - 1 ) * size. In conjunction with cb_push_back() you are allocating one element too much (or you are not using the last element of the ringbuffer). cb_count gets increased in every push_back too, so your buffer can have more "counts" than elements.

Improve this answer

1 Comment

btw. push and pop are both not safe. You probably want to check first and read/write afterwards.
0

If you are going to code in C++, at least use STL. Try std::list

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.