Geis

Merge lp://staging/~oif-team/geis/event-control-functions into lp://staging/geis

  1. event-control-functions
  2. Merge into trunk
Proposed by Stephen M. Webb
Status: Merged
Merged at revision: 93
Proposed branch: lp://staging/~oif-team/geis/event-control-functions
Merge into: lp://staging/geis
Prerequisite: lp://staging/geis/2.x
Diff against target: 1306 lines (+1032/-24)
18 files modified
.bzrignore (+7/-7)
ChangeLog (+55/-0)
include/geis/geis.h (+161/-1)
libutouch-geis/Makefile.am (+3/-1)
libutouch-geis/geis.c (+165/-10)
libutouch-geis/geis_attr.c (+16/-0)
libutouch-geis/geis_attr.h (+5/-0)
libutouch-geis/geis_event.c (+124/-0)
libutouch-geis/geis_event.h (+41/-0)
libutouch-geis/geis_event_queue.c (+154/-0)
libutouch-geis/geis_event_queue.h (+86/-0)
libutouch-geis/geis_private.h (+14/-1)
libutouch-geis/libutouch-geis.ver (+3/-0)
testsuite/libutouch-geis/Makefile.am (+2/-0)
testsuite/libutouch-geis/check_attr.c (+27/-0)
testsuite/libutouch-geis/check_backend_event_posting.c (+70/-0)
testsuite/libutouch-geis/check_event_queue.c (+88/-0)
testsuite/libutouch-geis/check_geis2_internals.c (+11/-4)
To merge this branch: bzr merge lp://staging/~oif-team/geis/event-control-functions
Reviewer Review Type Date Requested Status
Chase Douglas (community) Approve
Henrik Rydberg (community) Needs Information
Review via email: mp+43311@code.staging.launchpad.net

This proposal supersedes a proposal from 2010-12-05.

Description of the change

o adds a basic GeisEvent module

o adds an internal event queue module

o adds the event control functions to the Geis module

This resubmission incorporates Chase's requested changes (except #2, which was unclear).

This resubmission also incorporates a design change in the internal queueing (separate input and output queues) that came out of work on bug #674682. Better to put it in now on first commit than commit a known flaw and change it again later.

To post a comment you must log in.
Revision history for this message
Chase Douglas (chasedouglas) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Henrik Rydberg (rydberg) wrote :

There is a geis_event_attr_by_name(), but is there a way to retrieve the index corresponding to the name?

It would be good if the geis_event_queue_dequeue() docs state who owns the pointer after the call, and/or for how long the pointer is valid.

review: Needs Information
lp://staging/~oif-team/geis/event-control-functions updated
116. By Stephen M. Webb

Renamed event queue functions.

Revision history for this message
Stephen M. Webb (bregma) wrote :

> There is a geis_event_attr_by_name(), but is there a way to retrieve the index corresponding to
> the name?

No, the GeisEvent is effectively an associative container of attributes and I am not aware of any associative container that provides such functionality. Is there a use case you're thinking of?

> It would be good if the geis_event_queue_dequeue() docs state who owns the pointer after the call,
> and/or for how long the pointer is valid.

OK, I updated the docs to try to explain that the container uses value semantics on the opaque objects and does not control use of those object either before or after their being contained.

Revision history for this message
Henrik Rydberg (rydberg) wrote :

On 12/13/2010 03:41 PM, Stephen M. Webb wrote:

>> There is a geis_event_attr_by_name(), but is there a way to retrieve the index corresponding to
>> the name?
>
> No, the GeisEvent is effectively an associative container of attributes

> and I am not aware of any associative container that provides such
> functionality. Is there a use case you're thinking of?

Attributes are being read in abundance in the inner event loop, so some cpu will
be used for the lookup. Since there are two lookup methods, it seems the mapping
between the two is missing, or did I misread something? Usecase-wise, I would
collect the indices of the attributes in question at init, and then use the
indices during event handling. But sure, things will still work as they are,
only less effectively.

Thanks!

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

I didn't realize the merge proposal had been updated :(.

After reviewing the code again, I realized that GeisEvent is an opaque struct pointer, thus there's no copy overhead I mentioned in my previous review.

The only concern I had that isn't addressed yet is the #if 0. I still would like to see this fixed in time, but it's not worth holding this merge up.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: