feat(turbopack): turbo-esregex support captures method #81119
Conversation
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
2 similar comments
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
CodSpeed Performance ReportMerging #81119 will not alter performanceComparing Summary
Benchmarks breakdown
|
| match &self.delegate { | ||
| EsRegexImpl::Regex(r) => r.captures(haystack).map(|caps| { | ||
| caps.iter() | ||
| .map(|m| m.map(|m| m.as_str().to_string()).unwrap_or_default()) |
There was a problem hiding this comment.
I think you can avoid the .to_string() conversion and make the return type of this function Option<Vec<&'h str>> (where 'h is the lifetime of haystack). It seems like both regex and regress support such an API. The captures are just slices of the haystack.
| EsRegexImpl::Regex(r) => r.captures(haystack).map(|caps| { | ||
| caps.iter() | ||
| .map(|m| m.map(|m| m.as_str().to_string()).unwrap_or_default()) | ||
| .collect() |
There was a problem hiding this comment.
Optional: Instead of collect()ing here, it might be neat to try to make the return type an impl Iterator:
pub fn captures<'h>(&self, haystack: &'h str) -> Option<impl Iterator<&'h str>>
The iterator types for regex and regress are different, but we often use the either crate to work around this:
| let group_result = | ||
| std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { | ||
| m.group(group_index) | ||
| })); |
There was a problem hiding this comment.
We absolutely cannot do this:
- Panic handling in Rust is very slow, and should only be used for codepaths you expect to never be hit. This is especially true for library code (which this is).
- Panics (even if caught) trigger the global panic hooks, which we use in Next.js for things like cache invalidation and anonymized telemetry.
It looks like regress::Match has a groups function that returns an iterator: https://docs.rs/regress/latest/regress/struct.Match.html#method.groups
Could you use that?
EsRegex support captures method, in utoo situation, we need Esregexp to support captures the match string from user's regex config item.