Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently
vec::Iterstops if it yieldsvec.count()elements. This is incorrect in that it may not yield elements whose initialization was synchronized on the creation of the iterator, as we may read an out-of-datevec.count()while also have yielded an element that we didn't expect to (one that was concurrently initialized in-between elements that we meant to have yielded). This PR simplifies the iterator semantics and makes it completely fused on creation, whereas it previously may have continued to yield elements as they were added. I also simplified the implementation which may slightly hurt performance, but I think this will be fixed by #31 anyways (which from a cursory look correctly handles these issues). This also fixes another bug where the index was not updated if we skip over an uninitialized bucket, though that would be extremely rare.I think this was the underlying cause of astral-sh/ty#115 (comment), because Salsa relies on the iterator being linearizable, in particular,
vec.push(x); vec.iter().find(x).is_some();should never fail.