Side navigation
#447 closed bug (fixed)
Opened November 28, 2006 05:22PM UTC
Closed January 02, 2007 05:55PM UTC
Last modified June 19, 2007 07:00AM UTC
Refactoring of the filter function
Reported by: | andrea ercolino | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | 1.1a |
Component: | core | Version: | 1.1a |
Keywords: | refactoring, filter | Cc: | |
Blocked by: | Blocking: |
Description
I've refactored the "filter" core function of jQuery, merging my Chili parser into it. Now it should be cleaner, faster and tighter.
Main changes:
(*) a changed parsing engine where only 1 regexp match is executed on any call (there were a bunch more)
(*) a new preprocessing phase which builds one regexp out of the configuration object (is executed only on the first call, and is cached for later use)
(*) a changed configuration object (there were 2), which holds the regular expressions together with relative checks on the elements
(*) a tighter grammar, preserving its ease of definition
(*) comments, meaningful names
(*) fixed many bugs in the regexps
Attachments (0)
Change History (8)
Changed November 28, 2006 05:28PM UTC by comment:1
Changed November 28, 2006 05:35PM UTC by comment:2
component: | ajax → core |
---|
Changed December 04, 2006 02:26PM UTC by comment:3
No feedback?
Changed December 07, 2006 12:21PM UTC by comment:4
WARNING: This Wiki is messing up the regexps in this message
In my refactoring, there is a bug in this line:
step.len = 1 + (exp.replace(/\\\\|\\(/g, "").match(/((?!?)/g) || "" ).length;
the correct line should read:
step.len = 1 + (exp.replace( /\\./g, "%" ).replace( /[.*?]/g, "%" ).match( /((?!?)/g )|| []).length
I posted an article about this issue.
Changed December 11, 2006 08:18AM UTC by comment:5
Does this refactoring also handle the problems described in bug #453 (http://jquery.com/dev/bugs/bug/453/)? As not() uses filter(), fixing the problem in filter() should also fix not()...
I also wonder if you can find a way to replace/remove the old regexes completely. Currently that block at the end looks quite ugly.
Changed December 11, 2006 06:37PM UTC by comment:6
As Renato said in #453, the problem is that jQuery.filter does not support multiple selectors, because it matches from the beginning of the target, does the filtering, erases the recognized token in the target, and then starts another loop on the changed target. And as soon as the first selector is completely recognized (in n loops), the target becomes something that starts with a comma, which does not match anything, and filter ends.
A solution to #453 and the more general "multiple selectors" issue should be implemented inside $().not() and $().filter(). It shouldn't be too hard to explode on the comma in the target, filter by any piece, and finally add all up.
As for the old regexps at the bottom of the refactoring, surely they can be completely removed if the new interface in case of failure is considered acceptable. It's also very simple: remove all the "if( typeof matches == "undefined" ) {...}".
Note that I'm talking about interface in case of failure here, meaning that in case of success (a match is found between the regexps and the selector) the interface is preserved (filter works as it is supposed to). So the problem is really marginal, and I cannot think of someone in need of the old inconsistent interface. I provided that last check only to be rigorous about the refactoring.
The OLD interface in case of failure is:
jQuery.filter( target, result ) = { r: [], t: changed_target }
The NEW interface in case of failure is:
jQuery.filter( target, result ) = { r: result, t: target }
Changed December 23, 2006 05:25PM UTC by comment:7
priority: | major → minor |
---|---|
version: | 1.1 |
I think we're going to hold off on a complete refactor of the filter function, for now. (Considering that a complete speed overhaul was just finished.)
I recommend that you bring the refactoring up on the dev mailing list:
http://jquery.com/mailman/listinfo/dev_jquery.com
Some things that would help would be to point out any decreases in file size and any increases in speed that your patch provides.
Changed January 02, 2007 05:55PM UTC by comment:8
milestone: | → 1.1 |
---|---|
resolution: | → fixed |
status: | new → closed |
version: | → 1.1 |
Ok, I've integrated the IE speed fixes that you mentioned - and its helped a ton. I'm going to hold off on a complete re-org of filter for now, but your code is definitely pointing me in the right direction. Thanks again for all your help.
Please change Component: ajax to core. Thanks!