[PATCH 20/20] dix: reduce scope of tmp and mult.

[Peter Hutterer]

On Thu, Apr 21, 2011 at 11:58:03PM +0200, Simon Thum wrote:
> On 04/20/2011 10:21 AM, Daniel Stone wrote:
> > Hi,
> > 
> > On Wed, Apr 20, 2011 at 04:28:29PM +1000, Peter Hutterer wrote:
> >> index 0741604..4901d6a 100644
> >> --- a/dix/ptrveloc.c
> >> +++ b/dix/ptrveloc.c
> >> @@ -1122,7 +1122,6 @@ acceleratePointerPredictable(
> >>      ValuatorMask* val,
> >>      CARD32 evtime)
> >>  {
> >> -    float tmp, mult; /* no need to init */
> >>      int dx = 0, dy = 0, tmpi;
> >>      DeviceVelocityPtr velocitydata = GetDevicePredictableAccelData(dev);
> >>      Bool soften = TRUE;
> >> @@ -1150,6 +1149,8 @@ acceleratePointerPredictable(
> >>          }
> >>  
> >>          if (dev->ptrfeed && dev->ptrfeed->ctrl.num) {
> >> +            float mult;
> > 
> > Couldn't you just put tmp here as well, rather than put it twice below:
> > 
> >> @@ -1167,6 +1168,7 @@ acceleratePointerPredictable(
> >>                  /* Calculate the new delta (with accel) and drop it back
> >>                   * into the valuator masks */
> >>                  if (dx) {
> >> +                    float tmp;
> >>                      tmp = mult * fdx + dev->last.remainder[0];
> >>                      /* Since it may not be apparent: lrintf() does not offer
> >>                       * strong statements about rounding; however because we
> >> @@ -1178,6 +1180,7 @@ acceleratePointerPredictable(
> >>                      dev->last.remainder[0] = tmp - (float)tmpi;
> >>                  }
> >>                  if (dy) {
> >> +                    float tmp;
> >>                      tmp = mult * fdy + dev->last.remainder[1];
> >>                      tmpi = lrintf(tmp);
> >>                      valuator_mask_set(val, 1, tmpi);
> > 
> > But it doesn't matter really.  With or without that, for the series:
> > Reviewed-by: Daniel Stone <daniel at fooishbar.org>
> > 
> > I'd also found and written 01/20 independently (it's sitting in another
> > branch) while having a similar 'wtf is all this?' excursion through the
> > pointer code, and had wondered how it worked!
> Well, beyond a certain point there's simply no way I can make it better
> understandable for anyone but me in my current (at the time) mindset.
> Yes, it's complicated, but it's on par with the problem. And if I had
> gotten requests to clarify things in time, that might have been avoidable.

tbh, while I found it hard to get into the code, I couldn't find anything
major to simplify either without cutting the actual accel mechanism. Which
is why this patch series was little more than cosmetic changes.  Perhaps
the one thing that could aid in simplification is to slash Schemes and
replace them with profiles. Schemes seem a bit overengineered. But then
again, besides adding a few function pointers they don't really get in the
way either.

Some more documentation would have helped, I found it hard to get the big
picture in my head even with the wiki page. There are too many details that
weren't clear immediately and having what's in your brain about the accel
methods backed up somewhere would be a benefit :)

Cheers,
  Peter

> After all, the development process and review density surely improved.
> Today, some of peter's changes might have surfaced before even hitting
> master. But the real point is that if the issue is complex, the code
> will be too or it's too simple, meaning plain wrong.
> 
> I predict that X input will have to go some way towards more complexity
> to deliver the features one might naturally expect from a window system.
> There simply isn't much to argue about touchscreens bound to their
> output screen. X knows both. It's natural, it just requires some amount
> of in-server complexity - not some helper program floating around and
> fighting against other helper programs trying to achieve something else,
> piling up complexity on the system boundaries.
> 
> What can be argued about is reducible complexity, which means the scheme
> code here in accel. But this is the most straightforward part if you
> haven't been wading through mickeys before.
> 
> We can remove some of the settings and the scheme code without impairing
> functionality. We can make it simpler and better documented.
> 
> I'm happy to review patches.
> 
> Regards,
> 
> Simon