On Fri, 3 Jun 2011 16:30:26 +1000 Peter Hutterer <[email protected]> wrote:
> a few high-level comments, the patch itself looks sane on a first glance. > - let's just use "regex:", not either of "regex:" or "re:". one is enough. OK > - I'm not a big fan of allowing a mix of multiple arguments and |. We could > just require escaping | in a regex so that the result is a > MatchProduct "foo|!bar|regex:^blah$|regex:a\|b" > imo this is much easier to read than a multi-argument mix like your two > examples above. The string "\|" is itself a character '|' in RE, and "\\" is '\'. To use "\|" as '|', we have to write "\\|" and "\\\\" for characters '|' and '\'. Isn't it too much to pay for single arguments? > - I don't think we should expose the multi-match modes. As I said in a > previous email - if we have true regex support I think we can emulate all > of them with a regex. fnmatch etc. will still need to be used internally > for the native matching mode. OK, but it was just everything ready for the multi-match modes, if we switch from magic chars to mode numbers. > - man page additions please OK > Also, it'd be awesome if you could split the patch up into patches that > change the architecture and patches that then introduce the new > functionality. Makes it much easier to review and to find bugs through > bisection. I can see several separate patches in here - one to switch > over to use lists instead of char**, one to add the negation operator, one > to add regex support. If You don't mind, I'll show You a final sketch first, and, having it approved, submit it in three steps. Best regards, Oleh _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
