Code review comment for lp://staging/~oif-team/geis/event-control-functions

Revision history for this message
Chase Douglas (chasedouglas) wrote :

1. I would remove the geis_event_delete calls until it's implemented. I don't like seeing "#if 0" :). If you want to note something that should be changed in the future, I think a "FIXME: <blah>" is better. It gives context as to what is needed.

2. geis_event_queue_push allocates a GeisEvent on the stack and copies the event passed in. Then, the event is copied once more into the GeisEventQueueNode. The stack allocation and copying of the GeisEvent into the function is unnecessary. A pointer should be passed instead.

3. The event queue is a queue, not a stack. Queue semantics should be used, such as enqueue and dequeue. Using push and pop confuses me until I remember that it's a queue :).

4. geis_event_queue_front copies data into a geis event. Why not just return a pointer to the event? This would cut down on extra allocating and copying.

5. Imo, a cleaner interface is a dequeue function that returns the event if available and removes it from the queue. This requires one function call instead of two to retrieve the event and remove it.

review: Needs Fixing

« Back to merge proposal