On Thu, Jun 10, 2010 at 10:13 PM, Keith Packard <[email protected]> wrote: > > Argh! A recent commit to the xf86 config parsing code added "list.h" to > xf86Parser.h, which is included by drivers to look stuff up. Because of > this, the intel driver no longer builds against master (would that the > drivers were in the server tree...). I have an RC1 tar file sitting here > and decided that I really should be running those bits before pushing it > out, and I really think I should at least try running it first. > > To my mind, list.h really isn't needed in xf86Parser -- list elements > are only ever added to the head and then the whole list freed at once. > > Here's an open coded replacement; shorter and has no casts.
I've added some review if people want to go this route. > From 0192d160c51f58fbdf566ebf2b7e05faba51e91f Mon Sep 17 00:00:00 2001 > From: Keith Packard <[email protected]> > Date: Thu, 10 Jun 2010 22:08:46 -0700 > Subject: [PATCH] Use open-coded lists for Input match groups > > There's no reason to use the complicated list macros for something as > simple as a singly linked list; switching to open coded list walking > reduces the code considerably, and also eliminates exposing the list > macros in a header file required by drivers. > > Signed-off-by: Keith Packard <[email protected]> > --- > hw/xfree86/common/xf86Xinput.c | 24 +++--- > hw/xfree86/parser/InputClass.c | 205 > ++++++++++++++-------------------------- > hw/xfree86/parser/xf86Parser.h | 21 ++-- > 3 files changed, 91 insertions(+), 159 deletions(-) > > diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c > index 76d2d00..cd5f4ce 100644 > --- a/hw/xfree86/common/xf86Xinput.c > +++ b/hw/xfree86/common/xf86Xinput.c > @@ -555,13 +555,13 @@ match_path_pattern(const char *attr, const char > *pattern) > * If a pattern in each list entry is matched, return TRUE. > */ > static Bool > -MatchAttrToken(const char *attr, struct list *patterns, > +MatchAttrToken(const char *attr, xf86MatchGroup *patterns, > int (*compare)(const char *attr, const char *pattern)) > { > const xf86MatchGroup *group; > > /* If there are no patterns, accept the match */ > - if (list_is_empty(patterns)) > + if (!patterns) > return TRUE; > > /* If there are patterns but no attribute, reject the match */ > @@ -572,7 +572,7 @@ MatchAttrToken(const char *attr, struct list *patterns, > * Otherwise, iterate the list of patterns ensuring each entry has a > * match. Each list entry is a separate Match line of the same type. > */ > - list_for_each_entry(group, patterns, entry) { > + for (group = patterns; group; group = group->next) { > char * const *cur; > Bool match = FALSE; > > @@ -598,45 +598,45 @@ InputClassMatches(const XF86ConfInputClassPtr iclass, > const IDevPtr idev, > const InputAttributes *attrs) > { > /* MatchProduct substring */ > - if (!MatchAttrToken(attrs->product, &iclass->match_product, > match_substring)) > + if (!MatchAttrToken(attrs->product, iclass->match_product, > match_substring)) > return FALSE; > > /* MatchVendor substring */ > - if (!MatchAttrToken(attrs->vendor, &iclass->match_vendor, > match_substring)) > + if (!MatchAttrToken(attrs->vendor, iclass->match_vendor, > match_substring)) > return FALSE; > > /* MatchDevicePath pattern */ > - if (!MatchAttrToken(attrs->device, &iclass->match_device, > match_path_pattern)) > + if (!MatchAttrToken(attrs->device, iclass->match_device, > match_path_pattern)) > return FALSE; > > /* MatchOS case-insensitive string */ > - if (!MatchAttrToken(HostOS(), &iclass->match_os, strcasecmp)) > + if (!MatchAttrToken(HostOS(), iclass->match_os, strcasecmp)) > return FALSE; > > /* MatchPnPID pattern */ > - if (!MatchAttrToken(attrs->pnp_id, &iclass->match_pnpid, match_pattern)) > + if (!MatchAttrToken(attrs->pnp_id, iclass->match_pnpid, match_pattern)) > return FALSE; > > /* MatchUSBID pattern */ > - if (!MatchAttrToken(attrs->usb_id, &iclass->match_usbid, match_pattern)) > + if (!MatchAttrToken(attrs->usb_id, iclass->match_usbid, match_pattern)) > return FALSE; > > /* MatchDriver string */ > - if (!MatchAttrToken(idev->driver, &iclass->match_driver, strcmp)) > + if (!MatchAttrToken(idev->driver, iclass->match_driver, strcmp)) > return FALSE; > > /* > * MatchTag string > * See if any of the device's tags match any of the MatchTag tokens. > */ > - if (!list_is_empty(&iclass->match_tag)) { > + if (iclass->match_tag) { > char * const *tag; > Bool match; > > if (!attrs->tags) > return FALSE; > for (tag = attrs->tags, match = FALSE; *tag; tag++) { > - if (MatchAttrToken(*tag, &iclass->match_tag, strcmp)) { > + if (MatchAttrToken(*tag, iclass->match_tag, strcmp)) { > match = TRUE; > break; > } > diff --git a/hw/xfree86/parser/InputClass.c b/hw/xfree86/parser/InputClass.c > index ce611d9..95a4431 100644 > --- a/hw/xfree86/parser/InputClass.c > +++ b/hw/xfree86/parser/InputClass.c > @@ -66,14 +66,28 @@ xf86ConfigSymTabRec InputClassTab[] = > #define TOKEN_SEP "|" > > static void > -add_group_entry(struct list *head, char **values) > +xf86addMatchGroupEntry(xf86MatchGroup **head, char **values) > { > xf86MatchGroup *group; > > group = malloc(sizeof(*group)); > if (group) { > group->values = values; > - list_add(&group->entry, head); > + group->next = *head; > + *head = group; > + } > +} > + > +static void > +xf86freeMatchGroup(xf86MatchGroup **head) > +{ > + char **value; > + xf86MatchGroup *group; > + while ((group = *head)) { Nitpick. Newline between declarations and code. > + *head = group->next; > + for (value = group->values; *value; value++) > + free(*value); > + free(group); > } > } > > @@ -86,14 +100,14 @@ xf86parseInputClassSection(void) > parsePrologue(XF86ConfInputClassPtr, XF86ConfInputClassRec) > > /* Initialize MatchGroup lists */ > - list_init(&ptr->match_product); > - list_init(&ptr->match_vendor); > - list_init(&ptr->match_device); > - list_init(&ptr->match_os); > - list_init(&ptr->match_pnpid); > - list_init(&ptr->match_usbid); > - list_init(&ptr->match_driver); > - list_init(&ptr->match_tag); > + ptr->match_product = NULL; > + ptr->match_vendor = NULL; > + ptr->match_device = NULL; > + ptr->match_os = NULL; > + ptr->match_pnpid = NULL; > + ptr->match_usbid = NULL; > + ptr->match_driver = NULL; > + ptr->match_tag = NULL; This is unnecessary. parsePrologue uses calloc. list_init was only used to initialize the list pointers to each other. > while ((token = xf86getToken(InputClassTab)) != ENDSECTION) { > switch (token) { > @@ -122,50 +136,50 @@ xf86parseInputClassSection(void) > case MATCH_PRODUCT: > if (xf86getSubToken(&(ptr->comment)) != STRING) > Error(QUOTE_MSG, "MatchProduct"); > - add_group_entry(&ptr->match_product, > - xstrtokenize(val.str, TOKEN_SEP)); > + xf86addMatchGroupEntry(&ptr->match_product, > + xstrtokenize(val.str, TOKEN_SEP)); Please no tabs. This file is clean with only 4 space indents. I'd really hate to go back to the weird emacs default of real tabs at 8 spaces when the indent is 4. > break; > case MATCH_VENDOR: > if (xf86getSubToken(&(ptr->comment)) != STRING) > Error(QUOTE_MSG, "MatchVendor"); > - add_group_entry(&ptr->match_vendor, > - xstrtokenize(val.str, TOKEN_SEP)); > + xf86addMatchGroupEntry(&ptr->match_vendor, > + xstrtokenize(val.str, TOKEN_SEP)); > break; > case MATCH_DEVICE_PATH: > if (xf86getSubToken(&(ptr->comment)) != STRING) > Error(QUOTE_MSG, "MatchDevicePath"); > - add_group_entry(&ptr->match_device, > - xstrtokenize(val.str, TOKEN_SEP)); > + xf86addMatchGroupEntry(&ptr->match_device, > + xstrtokenize(val.str, TOKEN_SEP)); > break; > case MATCH_OS: > if (xf86getSubToken(&(ptr->comment)) != STRING) > Error(QUOTE_MSG, "MatchOS"); > - add_group_entry(&ptr->match_os, > - xstrtokenize(val.str, TOKEN_SEP)); > + xf86addMatchGroupEntry(&ptr->match_os, > + xstrtokenize(val.str, TOKEN_SEP)); > break; > case MATCH_PNPID: > if (xf86getSubToken(&(ptr->comment)) != STRING) > Error(QUOTE_MSG, "MatchPnPID"); > - add_group_entry(&ptr->match_pnpid, > - xstrtokenize(val.str, TOKEN_SEP)); > + xf86addMatchGroupEntry(&ptr->match_pnpid, > + xstrtokenize(val.str, TOKEN_SEP)); > break; > case MATCH_USBID: > if (xf86getSubToken(&(ptr->comment)) != STRING) > Error(QUOTE_MSG, "MatchUSBID"); > - add_group_entry(&ptr->match_usbid, > - xstrtokenize(val.str, TOKEN_SEP)); > + xf86addMatchGroupEntry(&ptr->match_usbid, > + xstrtokenize(val.str, TOKEN_SEP)); > break; > case MATCH_DRIVER: > if (xf86getSubToken(&(ptr->comment)) != STRING) > Error(QUOTE_MSG, "MatchDriver"); > - add_group_entry(&ptr->match_driver, > - xstrtokenize(val.str, TOKEN_SEP)); > + xf86addMatchGroupEntry(&ptr->match_driver, > + xstrtokenize(val.str, TOKEN_SEP)); > break; > case MATCH_TAG: > if (xf86getSubToken(&(ptr->comment)) != STRING) > Error(QUOTE_MSG, "MatchTag"); > - add_group_entry(&ptr->match_tag, > - xstrtokenize(val.str, TOKEN_SEP)); > + xf86addMatchGroupEntry(&ptr->match_tag, > + xstrtokenize(val.str, TOKEN_SEP)); > break; > case MATCH_IS_KEYBOARD: > if (xf86getSubToken(&(ptr->comment)) != STRING) > @@ -234,12 +248,22 @@ xf86parseInputClassSection(void) > return ptr; > } > > +static void > +xf86printMatchGroup(FILE *cf, xf86MatchGroup *group, char *title) > +{ > + char *const *cur; > + for(; group; group = group->next) { Nitpick. Newline between declarations and code. > + fprintf(cf, "\t%-17.17s\"", title); Won't 17.17 truncate if you happen to pass a title longer than 17 characters? And to that effect, there should be a space before the escaped quote in case the title is at least 17 characters. I'd suggest: fprintf(cf, "\t%-16s \"", title); > + for (cur = group->values; *cur; cur++) > + fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP, > + *cur); > + fprintf(cf, "\"\n"); Extra indent. It looks like it's part of the loop, but it's not. > + } > +} > + > void > xf86printInputClassSection (FILE * cf, XF86ConfInputClassPtr ptr) > { > - const xf86MatchGroup *group; > - char * const *cur; > - > while (ptr) { > fprintf(cf, "Section \"InputClass\"\n"); > if (ptr->comment) > @@ -249,62 +273,14 @@ xf86printInputClassSection (FILE * cf, > XF86ConfInputClassPtr ptr) > if (ptr->driver) > fprintf(cf, "\tDriver \"%s\"\n", ptr->driver); > > - list_for_each_entry(group, &ptr->match_product, entry) { > - fprintf(cf, "\tMatchProduct \""); > - for (cur = group->values; *cur; cur++) > - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP, > - *cur); > - fprintf(cf, "\"\n"); > - } > - list_for_each_entry(group, &ptr->match_vendor, entry) { > - fprintf(cf, "\tMatchVendor \""); > - for (cur = group->values; *cur; cur++) > - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP, > - *cur); > - fprintf(cf, "\"\n"); > - } > - list_for_each_entry(group, &ptr->match_device, entry) { > - fprintf(cf, "\tMatchDevicePath \""); > - for (cur = group->values; *cur; cur++) > - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP, > - *cur); > - fprintf(cf, "\"\n"); > - } > - list_for_each_entry(group, &ptr->match_os, entry) { > - fprintf(cf, "\tMatchOS \""); > - for (cur = group->values; *cur; cur++) > - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP, > - *cur); > - fprintf(cf, "\"\n"); > - } > - list_for_each_entry(group, &ptr->match_pnpid, entry) { > - fprintf(cf, "\tMatchPnPID \""); > - for (cur = group->values; *cur; cur++) > - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP, > - *cur); > - fprintf(cf, "\"\n"); > - } > - list_for_each_entry(group, &ptr->match_usbid, entry) { > - fprintf(cf, "\tMatchUSBID \""); > - for (cur = group->values; *cur; cur++) > - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP, > - *cur); > - fprintf(cf, "\"\n"); > - } > - list_for_each_entry(group, &ptr->match_driver, entry) { > - fprintf(cf, "\tMatchDriver \""); > - for (cur = group->values; *cur; cur++) > - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP, > - *cur); > - fprintf(cf, "\"\n"); > - } > - list_for_each_entry(group, &ptr->match_tag, entry) { > - fprintf(cf, "\tMatchTag \""); > - for (cur = group->values; *cur; cur++) > - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP, > - *cur); > - fprintf(cf, "\"\n"); > - } > + xf86printMatchGroup(cf, ptr->match_product, "MatchProduct"); > + xf86printMatchGroup(cf, ptr->match_vendor, "MatchVendor"); > + xf86printMatchGroup(cf, ptr->match_device, "MatchDevicePath"); > + xf86printMatchGroup(cf, ptr->match_os, "MatchOS"); > + xf86printMatchGroup(cf, ptr->match_pnpid, "MatchPnPID"); > + xf86printMatchGroup(cf, ptr->match_usbid, "MatchUSBID"); > + xf86printMatchGroup(cf, ptr->match_driver, "MatchDriver"); > + xf86printMatchGroup(cf, ptr->match_tag, "MatchTag"); > > if (ptr->is_keyboard.set) > fprintf(cf, "\tIsKeyboard \"%s\"\n", > @@ -336,60 +312,17 @@ xf86freeInputClassList (XF86ConfInputClassPtr ptr) > XF86ConfInputClassPtr prev; > > while (ptr) { > - xf86MatchGroup *group, *next; > - char **list; > - > TestFree(ptr->identifier); > TestFree(ptr->driver); > > - list_for_each_entry_safe(group, next, &ptr->match_product, entry) { > - list_del(&group->entry); > - for (list = group->values; *list; list++) > - free(*list); > - free(group); > - } > - list_for_each_entry_safe(group, next, &ptr->match_vendor, entry) { > - list_del(&group->entry); > - for (list = group->values; *list; list++) > - free(*list); > - free(group); > - } > - list_for_each_entry_safe(group, next, &ptr->match_device, entry) { > - list_del(&group->entry); > - for (list = group->values; *list; list++) > - free(*list); > - free(group); > - } > - list_for_each_entry_safe(group, next, &ptr->match_os, entry) { > - list_del(&group->entry); > - for (list = group->values; *list; list++) > - free(*list); > - free(group); > - } > - list_for_each_entry_safe(group, next, &ptr->match_pnpid, entry) { > - list_del(&group->entry); > - for (list = group->values; *list; list++) > - free(*list); > - free(group); > - } > - list_for_each_entry_safe(group, next, &ptr->match_usbid, entry) { > - list_del(&group->entry); > - for (list = group->values; *list; list++) > - free(*list); > - free(group); > - } > - list_for_each_entry_safe(group, next, &ptr->match_driver, entry) { > - list_del(&group->entry); > - for (list = group->values; *list; list++) > - free(*list); > - free(group); > - } > - list_for_each_entry_safe(group, next, &ptr->match_tag, entry) { > - list_del(&group->entry); > - for (list = group->values; *list; list++) > - free(*list); > - free(group); > - } > + xf86freeMatchGroup(&ptr->match_product); > + xf86freeMatchGroup(&ptr->match_vendor); > + xf86freeMatchGroup(&ptr->match_device); > + xf86freeMatchGroup(&ptr->match_os); > + xf86freeMatchGroup(&ptr->match_pnpid); > + xf86freeMatchGroup(&ptr->match_usbid); > + xf86freeMatchGroup(&ptr->match_driver); > + xf86freeMatchGroup(&ptr->match_tag); > > TestFree(ptr->comment); > xf86optionListFree(ptr->option_lst); > diff --git a/hw/xfree86/parser/xf86Parser.h b/hw/xfree86/parser/xf86Parser.h > index 337ad07..6283167 100644 > --- a/hw/xfree86/parser/xf86Parser.h > +++ b/hw/xfree86/parser/xf86Parser.h > @@ -66,7 +66,6 @@ > > #include <X11/Xdefs.h> > #include "xf86Optrec.h" > -#include "list.h" > > #define HAVE_PARSER_DECLS > > @@ -339,9 +338,9 @@ typedef struct > } > xf86TriState; > > -typedef struct > +typedef struct _xf86MatchGroup > { > - struct list entry; > + struct _xf86MatchGroup *next; > char **values; > } > xf86MatchGroup; > @@ -351,14 +350,14 @@ typedef struct > GenericListRec list; > char *identifier; > char *driver; > - struct list match_product; > - struct list match_vendor; > - struct list match_device; > - struct list match_os; > - struct list match_pnpid; > - struct list match_usbid; > - struct list match_driver; > - struct list match_tag; > + xf86MatchGroup *match_product; > + xf86MatchGroup *match_vendor; > + xf86MatchGroup *match_device; > + xf86MatchGroup *match_os; > + xf86MatchGroup *match_pnpid; > + xf86MatchGroup *match_usbid; > + xf86MatchGroup *match_driver; > + xf86MatchGroup *match_tag; > xf86TriState is_keyboard; > xf86TriState is_pointer; > xf86TriState is_joystick; > -- > 1.7.1 Besides the list.h vs. open-coded list decision, it's a nice cleanup. With the couple minor corrections above, Reviewed-by: Dan Nicholson <[email protected]> -- Dan _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
