refactor(regular_expression): Refactor regexp-modifiers support#11142
refactor(regular_expression): Refactor regexp-modifiers support#11142
regexp-modifiers support#11142Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors regex modifier support to use bitflags, simplifies modifier parsing logic, and updates related AST tooling and documentation.
- Switches from arrays to
bitflagsfor representing modifiers inModifiers - Refactors
parse_modifiersto detect duplicates and overlaps via flag operations - Adds allocator/
ContentEqsupport forModifierand updates display logic - Updates README stage and adds
bitflagsdependency
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_regular_expression/src/parser/pattern_parser/pattern_parser_impl.rs | Replaced enabling/disabling arrays with bitflags, simplified error checks in parse_modifiers |
| crates/oxc_regular_expression/src/ast_impl/span.rs | Added ContentEq impl for Modifier |
| crates/oxc_regular_expression/src/ast_impl/mod.rs | Introduced allocator module |
| crates/oxc_regular_expression/src/ast_impl/display.rs | Updated Display impl to use Modifier::contains |
| crates/oxc_regular_expression/src/ast_impl/allocator.rs | Implemented CloneIn for Modifier |
| crates/oxc_regular_expression/src/ast.rs | Defined Modifier via bitflags! and added alias for ast_tools |
| crates/oxc_regular_expression/README.md | Changed proposal stage from 3 to 4 |
| crates/oxc_regular_expression/Cargo.toml | Added bitflags dependency |
Comments suppressed due to low confidence (3)
crates/oxc_regular_expression/src/parser/pattern_parser/pattern_parser_impl.rs:1583
- [nitpick] The variable name
duplicateis ambiguous; consider renaming it tohas_duplicate_flagorduplicate_flag_detectedfor clarity.
let mut duplicate = false;
crates/oxc_regular_expression/src/parser/pattern_parser/pattern_parser_impl.rs:1651
- [nitpick] This compound
ifcondition combines multiple checks; consider extracting each check into well-named helper functions or separateifstatements to improve readability and maintainability.
if enabling.is_empty() && disabling.is_empty() || duplicate || [ast::Modifier::I, ast::Modifier::M, ast::Modifier::S].iter().any(|&modifier| enabling.contains(modifier) && disabling.contains(modifier))
crates/oxc_regular_expression/src/parser/pattern_parser/pattern_parser_impl.rs:1578
- Consider adding unit tests for scenarios such as duplicate modifier flags and overlapping enabling/disabling flags to verify the new error conditions.
fn parse_modifiers(&mut self) -> Result<Option<ast::Modifiers>> {
CodSpeed Instrumentation Performance ReportMerging #11142 will not alter performanceComparing Summary
|
This comment was marked as resolved.
This comment was marked as resolved.
|
Nice! |
When I first implemented this proposal, it was Stage 3 and the specs were not even fixed.
But for now, this spec is in Stage 4 and has been cleared up, so I've updated it.
In the process, I have done some refactoring.
bitflagslikeRegExpFlagsast_toolsworksNOTE: The basic behavior remains unchanged.