On Thu, May 27, 2010 at 08:34:31AM -0700, Ping Cheng wrote: > Thank you for the review (for both patches). My reply is inline as usual. > > Ping > > On Wed, May 26, 2010 at 11:08 PM, Peter Hutterer > <[email protected]> wrote: > > On Tue, May 25, 2010 at 08:07:58PM -0700, Ping Cheng wrote: > >> Change b to button and oldButton to oldOption to avoid confusion. > > > >> Signed-off-by: Ping Cheng <[email protected]> > >> --- > >> src/wcmValidateDevice.c | 32 ++++++++++++++++---------------- > >> 1 files changed, 16 insertions(+), 16 deletions(-) > >> > >> diff --git a/src/wcmValidateDevice.c b/src/wcmValidateDevice.c > >> index 438cf38..c46043f 100644 > >> --- a/src/wcmValidateDevice.c > >> +++ b/src/wcmValidateDevice.c > >> @@ -385,8 +385,8 @@ int wcmParseOptions(LocalDevicePtr local, int > >> hotplugged) > >> { > >> WacomDevicePtr priv = (WacomDevicePtr)local->private; > >> WacomCommonPtr common = priv->common; > >> - char *s, b[12]; > >> - int i, oldButton; > >> + char *s, button[12]; > >> + int i, oldOption; > > > > > > also, I'd suggest moving b down (see below). > > We are on the same page for this one. I had the same idea when I made > the change. However, if we move it inside the for-loop, it would be > declared WCM_MAX_BUTTONS times. I don't like that.
the compiler optimises things like that away so this has little effect on the code other than improved error checking at compile- and debug time. generally I find that relying on the compiler for micro-optimization like this is much more reliable than relying on myself :) > >> WacomToolPtr tool = NULL; > >> WacomToolAreaPtr area = NULL; > >> > >> @@ -570,12 +570,12 @@ int wcmParseOptions(LocalDevicePtr local, int > >> hotplugged) > >> if (TabletHasFeature(common, WCM_TPC)) > >> common->wcmTPCButtonDefault = 1; > >> > >> - oldButton = xf86SetBoolOption(local->options, "TPCButton", > >> + oldOption = xf86SetBoolOption(local->options, "TPCButton", > >> common->wcmTPCButtonDefault); > > > > personally, I find oldOption to be more confusing than oldButton. How about > > just tpc_button? > > It is not the button that matters. It is what the variable really > represent that matters here. The oldBuuton is also used for: > > --------------------------------------------- > - oldButton = xf86SetBoolOption(local->options, "Capacity", > + oldOption = xf86SetBoolOption(local->options, "Capacity", > common->wcmCapacityDefault); > > if (hotplugged || IsTouch(priv)) > - common->wcmCapacity = oldButton; > - else if (oldButton != common->wcmCapacity) > + common->wcmCapacity = oldOption; > + else if (oldOption != common->wcmCapacity) > xf86Msg(X_WARNING, "%s: Touch Capacity option > can only be" > "set by a touch tool.\n", local->name); > } > @@ -612,12 +612,12 @@ int wcmParseOptions(LocalDevicePtr local, int > hotplugged) > * except when multi-touch is supported */ > common->wcmGestureDefault = 1; > > - oldButton = xf86SetBoolOption(local->options, "Gesture", > + oldOption = xf86SetBoolOption(local->options, "Gesture", > common->wcmGestureDefault); > ------------------------------------------ > > They are the reason that I made the change. Unless you'd like to > introduce another variable for those two cases, it is confusing either > way. For me, no matter they are buttons or Gesture/Capacity, they are > all options in general. urgh, yes. please add a separate variable for each. it helps reading the code and as with the above, the compiler will probably squash them together into one anyway. thanks. Cheers, Peter ------------------------------------------------------------------------------ _______________________________________________ Linuxwacom-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
