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

Add struct base enumerator for interval trees. #73877

Merged
merged 19 commits into from
Jun 7, 2024

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 6, 2024
@@ -9,7 +9,7 @@ namespace Microsoft.CodeAnalysis.Shared.Collections;

internal partial class MutableIntervalTree<T>
{
protected sealed class Node
internal sealed class Node
Copy link
Member Author

Choose a reason for hiding this comment

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

needed as it's part of the signature of the public GetEnumerator now (can't have generic struct enumerators without leaking details like this).

where TIntervalTree : IIntervalTree<T>
where TIntervalTreeWitness : struct, IIntervalTreeWitness<T, TIntervalTree, TNode>
{
private static readonly ObjectPool<Stack<TNode>> s_nodeStackPool = new(() => new(), 128, trimOnFree: false);

public static IEnumerator<T> GetEnumerator(TIntervalTree tree)
Copy link
Member Author

Choose a reason for hiding this comment

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

so it is nice that this becomes a struct-enumerator and has no more code for itself anymore.

}

public readonly void Dispose()
=> _pooledStack.Dispose();
Copy link
Member Author

Choose a reason for hiding this comment

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

this is fine on an unintialized _pooledStack value. Dispose for it checks if it actually has an object from the pool, and no-ops if not.

@@ -171,12 +115,15 @@ public static bool Any<TIntrospector>(TIntervalTree tree, int start, int length,
{
var witness = default(TIntervalTreeWitness);

if (start == int.MinValue && end == int.MaxValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

if (start == int.MinValue && end == int.MaxValue)

was this a bug from before or newly necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

No bug. But it allows GetEnumerator to work (it wants all nodes and doesn't need/want to pass in an introspector. (It can't either since the callers don't have one).

Copy link
Member Author

Choose a reason for hiding this comment

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

Note; i doc'ed this on Enumerator side so it can be clear that it is intentionally passing in the full span, but not bothering with any sort of introspector (since it doesn't have one).

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review June 6, 2024 22:40
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner June 6, 2024 22:40
Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

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

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants