-
-
Notifications
You must be signed in to change notification settings - Fork 733
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
[memory] Fix read_cstring trying to read too far #1112
Conversation
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 think this approach is not very optimal. I suggested several options but feel free to bring others as long as they don't impact the perf as much.
Also this needs to be tested.
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.
This needs to be tested to ensure no regression later on.
Also still curious about the perf impact, did you run any perf test?
I didn't tested yet, and I'm thinking of cases where this fix doesn't work so I'll probably rework this PR soon |
Awesome! If this can wait, we can collab on this more actively by end of July/August when things get quieter on my end. Otherwise I'll try to find time here and there to review what you've done. |
e057b6d
to
effaa16
Compare
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.
Looks good, comments might be nice to explain what you're doing with the masking
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.
Some minor nits before merge, but the rest looks ok
fix for #1055