Cleanup more (?:) from patterns
#196
Conversation
Following up on slevithan#164, this change prevents a `(?:)` from being inserted in the following places: * At the beginning of a non-capturing group (the end is already handled) * Before or after a `|` * At the beginning or the end of the pattern This solution isn't as complete as the one suggested in slevithan#179, but it's a decent stopgap.
|
Any thoughts on this one, @slevithan? I'd like to land it if there's no objections. |
src/xregexp.js
Outdated
| // No need to separate tokens if at the beginning of a non-capturing group | ||
| match.input.slice(match.index - 3, 3) === '(?:' || | ||
| // No need to separate tokens if at the beginning of a non-capturing group or lookahead | ||
| match.input.slice(match.index - 3, 3).match(/\(\?[:=!]/) || |
There was a problem hiding this comment.
What happens if match.index - 3 is e.g. -1? Even if this works, it's not precise in its meaning so might be better to clarify.
Also, this should be using nativ.match or preferably nativ.test (nativ.test.call(regex, str)), at least as long as those still exist. XRegExp never uses native regex methods directly since (until v4.0.0) the native methods could be overridden. As a minor optimization, the regex should start with ^ so it isn't tested at all of the string's character positions if it fails to match.
src/xregexp.js
Outdated
| match.input[match.index - 1] === '|' || | ||
| match.input[match.index + match[0].length] === '|' || | ||
| // No need to separate tokens if at the beginning or end of the pattern | ||
| match.input[match.index - 1] === undefined || |
There was a problem hiding this comment.
Can you just check the value of match.index - 1 directly, without involving match.input?
src/xregexp.js
Outdated
| match.input[match.index + match[0].length] === '|' || | ||
| // No need to separate tokens if at the beginning or end of the pattern | ||
| match.input[match.index - 1] === undefined || | ||
| match.input[match.index + match[0].length] === undefined || |
There was a problem hiding this comment.
Ditto. Should probably be comparing to the length of match.input rather than referencing a specific character in the string.
There was a problem hiding this comment.
Also fixed in ee766c2
Following up on #164, this
change prevents a
(?:)from being inserted in the following places:|This solution isn't as complete as the one suggested in
#179, but it's a decent
stopgap.