Skip to content
This repository was archived by the owner on Sep 26, 2023. It is now read-only.

Conversation

@efritz
Copy link
Contributor

@efritz efritz commented Aug 10, 2020

We do binary search on all positions for a package, which is unnecessary. As we traverse the AST we can only pass along the positions which are valid for the parent (children necessarily fall in the same bounds).

@efritz efritz requested review from aidaeology and garobrik August 10, 2020 19:15
Copy link
Contributor

@garobrik garobrik left a comment

Choose a reason for hiding this comment

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

LGTM

})

end := start
for end < len(positions) && positions[end] <= node.End() {
Copy link
Contributor

Choose a reason for hiding this comment

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

perf nit since it's a perf PR:
finding end would be faster using another application of sort.Search, but idk how many positions an ast node can span for whether it matters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My assumption is that it's fairly small and I don't want to break cache locality here.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense

Base automatically changed from hoist-preload-positions to master August 13, 2020 00:15
@efritz efritz merged commit bef4780 into master Aug 13, 2020
@efritz efritz deleted the cut-positions-during-traversal branch August 13, 2020 00:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants