include/list.h
Dan Nicholson
dbn.lists at gmail.com
Fri Jun 11 12:53:57 PDT 2010
On Thu, Jun 10, 2010 at 10:13 PM, Keith Packard <keithp at keithp.com> 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 <keithp at keithp.com>
> 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 <keithp at keithp.com>
> ---
> 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 <dbn.lists at gmail.com>
--
Dan
More information about the xorg-devel
mailing list