Re: [PATCH 01/11] dix: split out window owner event delivery from DeliverEventsToWindow

2011-05-11 Thread Jamey Sharp 
This looks good, but is there some strong reason not to just use early
returns in DeliverToWindowOwner?

I'd re-write it this way:

On Wed, May 11, 2011 at 02:49:40PM +1000, Peter Hutterer wrote:
 +static enum EventDeliveryState
 +DeliverToWindowOwner(DeviceIntPtr dev, WindowPtr win,
 + xEvent *events, int count, Mask filter,
 + GrabPtr grab)
 +{
 +int attempt;
 +enum EventDeliveryState rc = EVENT_SKIP;
 +
 +/* if nobody ever wants to see this event, skip some work */
 +if (filter != CantBeFiltered 
 +!((wOtherEventMasks(win)|win-eventMask)  filter))
 +goto out;
 +
 +if (IsInterferingGrab(wClient(win), dev, events))
 +goto out;
 +
 +rc = EVENT_NOT_DELIVERED;
 +
 +if (XaceHook(XACE_RECEIVE_ACCESS, wClient(win), win, events, count))
 +/* do nothing */;
 +else if ((attempt = TryClientEvents(wClient(win), dev, events,
 +count, win-eventMask,
 +filter, grab)))
 +rc = (attempt  0) ? EVENT_DELIVERED : EVENT_REJECTED;
 +
 +out:
 +return rc;
 +}

static enum EventDeliveryState
DeliverToWindowOwner(DeviceIntPtr dev, WindowPtr win,
 xEvent *events, int count, Mask filter,
 GrabPtr grab)
{
/* if nobody ever wants to see this event, skip some work */
if (filter != CantBeFiltered 
!((wOtherEventMasks(win)|win-eventMask)  filter))
return EVENT_SKIP;

if (IsInterferingGrab(wClient(win), dev, events))
return EVENT_SKIP;

if (!XaceHook(XACE_RECEIVE_ACCESS, wClient(win), win, events, count))
{
int attempt = TryClientEvents(wClient(win), dev, events,
  count, win-eventMask,
  filter, grab);

if (attempt  0)
return EVENT_DELIVERED;
if (attempt  0)
return EVENT_REJECTED;
}

return EVENT_NOT_DELIVERED;
}

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 01/11] dix: split out window owner event delivery from DeliverEventsToWindow

2011-05-11 Thread Peter Hutterer 
On Wed, May 11, 2011 at 01:36:49PM -0700, Jamey Sharp wrote:
 This looks good, but is there some strong reason not to just use early
 returns in DeliverToWindowOwner?
 
 I'd re-write it this way:
 
 On Wed, May 11, 2011 at 02:49:40PM +1000, Peter Hutterer wrote:
  +static enum EventDeliveryState
  +DeliverToWindowOwner(DeviceIntPtr dev, WindowPtr win,
  + xEvent *events, int count, Mask filter,
  + GrabPtr grab)
  +{
  +int attempt;
  +enum EventDeliveryState rc = EVENT_SKIP;
  +
  +/* if nobody ever wants to see this event, skip some work */
  +if (filter != CantBeFiltered 
  +!((wOtherEventMasks(win)|win-eventMask)  filter))
  +goto out;
  +
  +if (IsInterferingGrab(wClient(win), dev, events))
  +goto out;
  +
  +rc = EVENT_NOT_DELIVERED;
  +
  +if (XaceHook(XACE_RECEIVE_ACCESS, wClient(win), win, events, count))
  +/* do nothing */;
  +else if ((attempt = TryClientEvents(wClient(win), dev, events,
  +count, win-eventMask,
  +filter, grab)))
  +rc = (attempt  0) ? EVENT_DELIVERED : EVENT_REJECTED;
  +
  +out:
  +return rc;
  +}
 
 static enum EventDeliveryState
 DeliverToWindowOwner(DeviceIntPtr dev, WindowPtr win,
  xEvent *events, int count, Mask filter,
  GrabPtr grab)
 {
 /* if nobody ever wants to see this event, skip some work */
 if (filter != CantBeFiltered 
 !((wOtherEventMasks(win)|win-eventMask)  filter))
 return EVENT_SKIP;
 
 if (IsInterferingGrab(wClient(win), dev, events))
 return EVENT_SKIP;
 
 if (!XaceHook(XACE_RECEIVE_ACCESS, wClient(win), win, events, count))
 {
 int attempt = TryClientEvents(wClient(win), dev, events,
   count, win-eventMask,
   filter, grab);
 
 if (attempt  0)
 return EVENT_DELIVERED;
 if (attempt  0)
 return EVENT_REJECTED;
 }
 
 return EVENT_NOT_DELIVERED;
 }
 

I'm a big fan of the goto out because it makes debugging so much easier.
setting a breakpoint or adding a printf at a single point is faster than
remembering to set 5 points. That's pretty much the only reason for me.

Cheers,
  Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 01/11] dix: split out window owner event delivery from DeliverEventsToWindow

2011-05-11 Thread Jamey Sharp 
On Thu, May 12, 2011 at 10:33:43AM +1000, Peter Hutterer wrote:
 I'm a big fan of the goto out because it makes debugging so much easier.
 setting a breakpoint or adding a printf at a single point is faster than
 remembering to set 5 points. That's pretty much the only reason for me.

I see your point, and raise you two counter-points:

This function is only called in one place. You can set a breakpoint or
add a printf there and catch all return paths in either style. (For that
matter, gdb's finish command will tell you what the return value was
no matter which path the function exited through, so you only need a
breakpoint on function entry...)

In my opinion, the control flow in this function is so much easier to
read and reason about with early-return that any convenience in the
(hopefully rare!) case of needing to debug it is outweighed anyway.

I believe you that there are times when early return is the wrong
choice, but I don't think this is one of them.

Jamey


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 01/11] dix: split out window owner event delivery from DeliverEventsToWindow

2011-05-11 Thread Peter Hutterer 
On Wed, May 11, 2011 at 05:51:50PM -0700, Jamey Sharp wrote:
 On Thu, May 12, 2011 at 10:33:43AM +1000, Peter Hutterer wrote:
  I'm a big fan of the goto out because it makes debugging so much easier.
  setting a breakpoint or adding a printf at a single point is faster than
  remembering to set 5 points. That's pretty much the only reason for me.
 
 I see your point, and raise you two counter-points:

doh. Now I need to come up with more counter points to match your offer and
raise the stakes :)

 This function is only called in one place. You can set a breakpoint or
 add a printf there and catch all return paths in either style. (For that
 matter, gdb's finish command will tell you what the return value was
 no matter which path the function exited through, so you only need a
 breakpoint on function entry...)

finish is great for rarely called functions, bad for anything in the event
path that gets hammered whenever you do so much as think of the mouse.

 In my opinion, the control flow in this function is so much easier to
 read and reason about with early-return that any convenience in the
 (hopefully rare!) case of needing to debug it is outweighed anyway.
 
 I believe you that there are times when early return is the wrong
 choice, but I don't think this is one of them.

damn. couldn't come up with another point. ok, you win.

Cheers,
  Peter
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 01/11] dix: split out window owner event delivery from DeliverEventsToWindow

2011-05-10 Thread Peter Hutterer 
No functional changes, just for readability.

Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
---
 dix/events.c |   78 +++---
 1 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/dix/events.c b/dix/events.c
index 276bc75..92fd41d 100644
--- a/dix/events.c
+++ b/dix/events.c
@@ -1939,6 +1939,46 @@ TryClientEvents (ClientPtr client, DeviceIntPtr dev, 
xEvent *pEvents,
 return 1;
 }
 
+enum EventDeliveryState {
+EVENT_DELIVERED, /** Event has been delivered to a client  */
+EVENT_NOT_DELIVERED, /** Event was not delivered to any client */
+EVENT_SKIP,  /** Event can be discarded by the caller  */
+EVENT_REJECTED,  /** Event was rejected for delivery to the client */
+};
+
+/**
+ * Attempt event delivery to the client owning the window.
+ */
+static enum EventDeliveryState
+DeliverToWindowOwner(DeviceIntPtr dev, WindowPtr win,
+ xEvent *events, int count, Mask filter,
+ GrabPtr grab)
+{
+int attempt;
+enum EventDeliveryState rc = EVENT_SKIP;
+
+/* if nobody ever wants to see this event, skip some work */
+if (filter != CantBeFiltered 
+!((wOtherEventMasks(win)|win-eventMask)  filter))
+goto out;
+
+if (IsInterferingGrab(wClient(win), dev, events))
+goto out;
+
+rc = EVENT_NOT_DELIVERED;
+
+if (XaceHook(XACE_RECEIVE_ACCESS, wClient(win), win, events, count))
+/* do nothing */;
+else if ((attempt = TryClientEvents(wClient(win), dev, events,
+count, win-eventMask,
+filter, grab)))
+rc = (attempt  0) ? EVENT_DELIVERED : EVENT_REJECTED;
+
+out:
+return rc;
+}
+
+
 /**
  * Deliver events to a window. At this point, we do not yet know if the event
  * actually needs to be delivered. May activate a grab if the event is a
@@ -1975,28 +2015,26 @@ DeliverEventsToWindow(DeviceIntPtr pDev, WindowPtr 
pWin, xEvent
 /* Deliver to window owner */
 if ((filter == CantBeFiltered) || CORE_EVENT(pEvents))
 {
-   /* if nobody ever wants to see this event, skip some work */
-   if (filter != CantBeFiltered 
-   !((wOtherEventMasks(pWin)|pWin-eventMask)  filter))
-   return 0;
+enum EventDeliveryState rc;
 
-if (IsInterferingGrab(wClient(pWin), pDev, pEvents))
-return 0;
+rc = DeliverToWindowOwner(pDev, pWin, pEvents, count, filter, grab);
 
-   if (XaceHook(XACE_RECEIVE_ACCESS, wClient(pWin), pWin, pEvents, count))
-   /* do nothing */;
-else if ( (attempt = TryClientEvents(wClient(pWin), pDev, pEvents,
- count, pWin-eventMask,
- filter, grab)) )
-   {
-   if (attempt  0)
-   {
-   deliveries++;
-   client = wClient(pWin);
-   deliveryMask = pWin-eventMask;
-   } else
-   nondeliveries--;
-   }
+switch(rc)
+{
+case EVENT_SKIP:
+return 0;
+case EVENT_REJECTED:
+nondeliveries--;
+break;
+case EVENT_DELIVERED:
+/* We delivered to the owner, with our event mask */
+deliveries++;
+client = wClient(pWin);
+deliveryMask = pWin-eventMask;
+break;
+case EVENT_NOT_DELIVERED:
+break;
+}
 }
 
 /* CantBeFiltered means only window owner gets the event */
-- 
1.7.4.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel