-
Notifications
You must be signed in to change notification settings - Fork 784
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
Arbitrary size concat elements utf8 #1787
Arbitrary size concat elements utf8 #1787
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1787 +/- ##
=======================================
Coverage 83.47% 83.48%
=======================================
Files 221 221
Lines 57042 57094 +52
=======================================
+ Hits 47616 47665 +49
- Misses 9426 9429 +3
Continue to review full report at Codecov.
|
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.
I would be very interested to see some benchmarks of this, this has introduced a number of additional memory allocations and a significantly more complicated loop body, it may not matter, but it would be good to verify either way...
FYI I think @tustvold is out this week Another way to move this PR forward might be to make this a new kernel (rather than modifying the existing That way we aren't worried about a potential regression in performance and people can optimize the new kernel in the future if that is desired |
@Ismail-Maj This could be done as a follow-up PR. |
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.
Sorry for the delay, thank you 👍
Which issue does this PR close?
Closes #1748.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?
Should be merged after #1780.