-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve merging of Char parsers #291
Conversation
johnynek
commented
Oct 30, 2021
- If we have a single CharIn parser, we don't need to compute the union, just reuse it (which also saves recomputing the range of characters)
- return a Set in the union function, not a List, which can return a large list if you have duplicates in your set.
Codecov Report
@@ Coverage Diff @@
## main #291 +/- ##
==========================================
+ Coverage 95.98% 96.43% +0.45%
==========================================
Files 8 8
Lines 997 1011 +14
Branches 88 81 -7
==========================================
+ Hits 957 975 +18
+ Misses 40 36 -4
Continue to review full report at Codecov.
|
val minBs: List[(Int, BitSetUtil.Tpe)] = many.map { case CharIn(m, bs, _) => (m, bs) } | ||
Chain.one(Parser.charIn(BitSetUtil.union(minBs)).asInstanceOf[P0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we have a version of union
with Iterator
and avoid the copy in minBs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we certainly could. I wasn't sure it was worth it because we already iterate through the number of parsers regularly, but the characters inside the parsers (which we have to iterate when allocating a new CharIn to compute the ranges) could be more expensive, which was what I was targeting here.
Do you want me to address this before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can address this as a follow-up for sure!
Co-authored-by: Filipe Regadas <filiperegadas@gmail.com>