Skip to content
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

fix IB in fifoToOwnedArrayList #21404

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matklad
Copy link
Contributor

@matklad matklad commented Sep 14, 2024

memcpy requires non-overlapping arguments.

fifo.realign() handles this case correctly and tries to provide an optimized implementation.

This probably wasn't hit in practice, as, in a typical usage, fifo's head is not advanced.

memcpy requires non-overlapping arguments.

fifo.realign() handles this case correctly and tries to provide an
optimized implementation.

This probably wasn't hit in practice, as, in a typical usage, fifo's
head is not advanced.
Copy link
Contributor

@daurnimator daurnimator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need the if (fifo.head != 0) check: realign should be cheap enough to do unconditionally.

@matklad
Copy link
Contributor Author

matklad commented Sep 16, 2024

Hm, if I am reading the code correctly, head==0 would go into this branch:

mem.copyForwards(T, self.buf[0..self.count], self.buf[self.head..][0..self.count]);

Which is a (noop) for-loop over the entire data, which feels like it could be costly, especially given that head==0 is the common case actually.

@daurnimator
Copy link
Contributor

Which is a (noop) for-loop over the entire data, which feels like it could be costly, especially given that head==0 is the common case actually.

sorry you're right!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants