[PATCH 1/3 v2'] xserver: Use lists and constants for matching modes in Match entries

Oleh R. Nykyforchyn oleh.nyk at gmail.com
Fri Jun 17 12:16:09 PDT 2011


On Fri, 17 Jun 2011 08:19:15 -0700
Dan Nicholson <dbn.lists at gmail.com> wrote:

> I'm not sure what to do here. I like the refactoring with the
> enumerated match types. However, I have a few issues with this.
> 
> 1. The commit subject talks about using lists and constants, but then
> there's other stuff like using multiple arguments and changing the
> parser that are unrelated.
Parser changes are directly related to lists because xstrtokenize() returns
char**, which is not what we need. AFAIK it is recommenged that a commit
subject is not longer than 70 (or so) chars.

> The "(implicit)" bugfix is completely
> unrelated and should be in a separate commit if it's even needed at
> all (see below).
"(implicit)" is already in the code, the patch just moves it into a proper place accordingly
to new code structure.

> 2. I still don't think anyone is clamoring to have multiple arguments
> when the same thing is allowed with '|'.
Almost the same, except mixed regex and non-regex patterns.

> People have been getting
> along pretty well so far without them. I'd rather keep it simple and
> just have one method to specify multiple values on one line.
Again, with ONE regex in line only.  My point is that "Freedom is the
UNIX way": if we can allow something, while preserving all old possibilities,
why don't do it?

> Furthermore, the implementation causes you to have lists within lists
> and makes the terminology (patterns vs. groups) even more difficult to
> understand than it is now.
I can't say about everybody, but it was not so obvious to me first what is xf86MatchGroup.
Group of what? Now it is a group of patterns. BTW, for a newcomer "pattern" is clearer
than "token", which seems to be introduced just because of strtok().
> 
> You've clearly spent a lot of time working on this, and there's some
> good stuff here, so I hope we can find a way to land it.
Thanks

Oleh


More information about the xorg-devel mailing list