[Merged by Bors] - Rework RegExp struct to include bitflags field #1837

aaronmunsters · 2022-02-13T17:04:26Z

This Pull Request fixes/closes #1819.

It changes the following:

Move the bitflags from boa/src/syntax/lexer/regex.rs to boa/src/builtins/regexp/mod.rs
Replace the booleans in the RegExp struct to include the bitflags struct
Update match expressions to make use of the bitflags struct

This commit fixes/closes boa-dev#1819.

codecov · 2022-02-13T17:21:04Z

Codecov Report

Merging #1837 (325cb85) into main (be26b10) will decrease coverage by 0.00%.
The diff coverage is 63.88%.

@@            Coverage Diff             @@
##             main    #1837      +/-   ##
==========================================
- Coverage   55.67%   55.67%   -0.01%     
==========================================
  Files         201      201              
  Lines       17375    17368       -7     
==========================================
- Hits         9674     9670       -4     
+ Misses       7701     7698       -3

Impacted Files	Coverage Δ
boa/src/syntax/lexer/mod.rs	`64.95% <ø> (ø)`
boa/src/builtins/regexp/mod.rs	`64.92% <40.90%> (-0.27%)`	⬇️
boa/src/syntax/lexer/regex.rs	`57.50% <100.00%> (+2.83%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be26b10...325cb85. Read the comment docs.

raskad · 2022-02-13T17:27:28Z

VM implementation

Test result	main count	PR count	difference
Total	87,912	87,912	0
Passed	40,892	40,892	0
Ignored	19,929	19,929	0
Failed	27,091	27,091	0
Panics	0	0	0
Conformance	46.51%	46.51%	0.00%

Razican

Looking very good! I added some suggestions :)

Razican · 2022-02-14T04:52:32Z

boa/src/builtins/regexp/mod.rs

-        let mut dot_all = false;
-        let mut unicode = false;
-        let mut sticky = false;
+        let mut flags = RegExpFlags::default();
        for c in f.chars() {


I would implement FromStr for RegexFlags, and use the version of parse_regex_flags() in the lexer, which iterates through bytes instead of characters, and should therefore be more efficient. Would also reduce duplicate code.

Razican · 2022-02-14T04:53:10Z

boa/src/builtins/regexp/mod.rs

-                    'i' => regexp.ignore_case,
-                    'u' => regexp.unicode,
-                    'y' => regexp.sticky,
+                    'g' => regexp.flags.contains(RegExpFlags::GLOBAL),


Could we match bytes instead of characters here? Should be more efficient

In my last commit the code matches on bytes, but this could be replaced with the RegExpFlags which would allow for the following reduction in matching code:

fn regexp_has_flag(this: &JsValue, flag: RegExpFlags, context: &mut Context) -> JsResult<JsValue> { if let Some(object) = this.as_object() { if let Some(regexp) = object.borrow().as_regexp() { return Ok(JsValue::new(regexp.flags.contains(flag))); } // ... } // ... }

Which would be more readable?

Razican · 2022-02-14T04:54:47Z

boa/src/syntax/lexer/regex.rs

-        const STICKY = 0b0010_0000;
-    }
-}
-
 fn parse_regex_flags(s: &str, start: Position, interner: &mut Interner) -> Result<Sym, Error> {


I would move this function together with the structure, and change it so that this implements the FromStr trait for the structure.

RageKnify · 2022-02-14T17:39:22Z

Don't agree with moving the struct and accompanying functions from its original place.
It works against the idea of having the interpreter/environment depend on other parts of boa, ideally we'll be able to make the lexer and/or parser be separate crates to reduce iterative compile times. As is this PR would introduce dependencies going the other way.
Other than this the PR lgtm

Razican · 2022-02-14T18:12:18Z

Don't agree with moving the struct and accompanying functions from its original place.

It works against the idea of having the interpreter/environment depend on other parts of boa, ideally we'll be able to make the lexer and/or parser be separate crates to reduce iterative compile times. As is this PR would introduce dependencies going the other way.

Other than this the PR lgtm

Well, ideally this structure would ideally be part of regress. But I see what you mean. Maybe it should stay in the lexer.

aaronmunsters · 2022-02-15T13:14:23Z

I reworked the PR with the provided feedback. Feel free to leave new suggestions :)

RageKnify

LGTM

Razican · 2022-02-15T20:44:25Z

This just needs running cargo fit

RageKnify

bors r+

This Pull Request fixes/closes #1819. It changes the following: - Move the bitflags from `boa/src/syntax/lexer/regex.rs` to `boa/src/builtins/regexp/mod.rs` - Replace the booleans in the RegExp struct to include the bitflags struct - Update match expressions to make use of the bitflags struct Co-authored-by: Aäron Munsters <[email protected]>

bors · 2022-02-16T13:12:21Z

Pull request successfully merged into main.

Build succeeded:

Rework RegExp struct to include bitflags field

f4601f0

This commit fixes/closes boa-dev#1819.

raskad added this to the v0.14.0 milestone Feb 13, 2022

raskad added builtins PRs and Issues related to builtins/intrinsics performance Performance related changes and issues labels Feb 13, 2022

Razican reviewed Feb 14, 2022

View reviewed changes

Razican requested review from raskad, HalidOdat, jasonwilliams, RageKnify and jedel1043 February 14, 2022 04:55

aaronmunsters and others added 4 commits February 15, 2022 10:39

Merge branch 'boa-dev:main' into rework_regexp_struct

1d81ed3

Move bitflags struct to lexer

2500e8d

Moved characters into bytes

b04df5f

Implement FromStr trait for RegExpFlags

1b64b2f

RageKnify approved these changes Feb 15, 2022

View reviewed changes

Razican approved these changes Feb 15, 2022

View reviewed changes

Update code to align with format

325cb85

RageKnify approved these changes Feb 16, 2022

View reviewed changes

bors bot changed the title ~~Rework RegExp struct to include bitflags field~~ [Merged by Bors] - Rework RegExp struct to include bitflags field Feb 16, 2022

bors bot closed this Feb 16, 2022

aaronmunsters deleted the rework_regexp_struct branch February 16, 2022 20:37

RageKnify added the Internal Category for changelog label Mar 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Rework RegExp struct to include bitflags field #1837

[Merged by Bors] - Rework RegExp struct to include bitflags field #1837

aaronmunsters commented Feb 13, 2022 •

edited

Loading

codecov bot commented Feb 13, 2022 •

edited

Loading

raskad commented Feb 13, 2022

Razican left a comment

Razican Feb 14, 2022

Razican Feb 14, 2022

aaronmunsters Feb 15, 2022

Razican Feb 14, 2022

RageKnify commented Feb 14, 2022

Razican commented Feb 14, 2022

aaronmunsters commented Feb 15, 2022

RageKnify left a comment

Razican commented Feb 15, 2022

RageKnify left a comment

bors bot commented Feb 16, 2022

[Merged by Bors] - Rework RegExp struct to include bitflags field #1837

[Merged by Bors] - Rework RegExp struct to include bitflags field #1837

Conversation

aaronmunsters commented Feb 13, 2022 • edited Loading

codecov bot commented Feb 13, 2022 • edited Loading

Codecov Report

raskad commented Feb 13, 2022

VM implementation

Razican left a comment

Choose a reason for hiding this comment

Razican Feb 14, 2022

Choose a reason for hiding this comment

Razican Feb 14, 2022

Choose a reason for hiding this comment

aaronmunsters Feb 15, 2022

Choose a reason for hiding this comment

Razican Feb 14, 2022

Choose a reason for hiding this comment

RageKnify commented Feb 14, 2022

Razican commented Feb 14, 2022

aaronmunsters commented Feb 15, 2022

RageKnify left a comment

Choose a reason for hiding this comment

Razican commented Feb 15, 2022

RageKnify left a comment

Choose a reason for hiding this comment

bors bot commented Feb 16, 2022

aaronmunsters commented Feb 13, 2022 •

edited

Loading

codecov bot commented Feb 13, 2022 •

edited

Loading