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.

Sign up for GitHub

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

Jump to bottom

Modernise several uses of GetSortedLanes() batch 2 #1496

Merged
merged 8 commits into from
Apr 3, 2022
Merged

Modernise several uses of GetSortedLanes() batch 2 #1496

merged 8 commits into from
Apr 3, 2022

Conversation

originalfoo
Copy link
Member

@originalfoo originalfoo commented Mar 28, 2022 •
edited

Another batch.

  • Add new CountLanes() extension
    • There were several places where GetSortedLanes() was being used just to count lanes so created an extension which does that more optimally
  • Modernise several GetSortedLanes() call sites

@originalfoo originalfoo added the code cleanup Refactor code, remove old code, improve maintainability label Mar 28, 2022
@originalfoo originalfoo added this to the 11.6.5.2 milestone Mar 28, 2022
@originalfoo originalfoo self-assigned this Mar 28, 2022
@originalfoo
Modernise several uses of GetSortedLanes()
e97c983 
- Add new `CountLanes()` extension
- Modernise several `GetSortedLanes()`
@originalfoo
Merge branch 'master' into sorted-lanes-modernisation2
c833d77
@originalfoo originalfoo changed the title Modernise several uses of GetSortedLanes() Modernise several uses of GetSortedLanes() batch 2 Mar 28, 2022
@originalfoo
Remove GetSortedLanes from CountLanes
aba2136 
Use specific code for counting lanes, rather than getting
`.Count` of a list of sorted lanes.
@originalfoo
Retain reverse param.
ffdb68c 
Not sure if it's needed here, but original code had it so keeping it just in case.
@originalfoo
Change function to static method.
633331f
@originalfoo
Merge branch 'master' into sorted-lanes-modernisation2
a842a0b
@originalfoo
Copy link
Member Author

bump

@originalfoo originalfoo added the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Apr 2, 2022
@originalfoo
Guard against null segment Info and Info.m_lanes
cc2dc85 
Based on checks used in similar vanilla methods.
@originalfoo
Copy link
Member Author

originalfoo commented Apr 2, 2022 •
edited

Added guard against null segment Info and Info.m_lanes based on vanilla CSL code.

@krzychu124 || @kianzarrin can you do a quick check of cc2dc85 just to make sure no issues before I merge?

@originalfoo originalfoo removed the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Apr 2, 2022
@originalfoo originalfoo merged commit c5f9786 into master Apr 3, 2022
@originalfoo originalfoo deleted the sorted-lanes-modernisation2 branch April 3, 2022 01:32
@kianzarrin
Copy link
Collaborator

Oh sorry I forgot to look at this.
We should generally use GetSegmentLaneIndexesAndIds() to avoid such mistakes. but that is out of scope of this PR

@originalfoo
Copy link
Member Author

@kianzarrin that's tracked in #1489

@originalfoo originalfoo mentioned this pull request Apr 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krzychu124 krzychu124 krzychu124 approved these changes

@kianzarrin kianzarrin kianzarrin approved these changes

@kvakvs kvakvs Awaiting requested review from kvakvs

@DaEgi01 DaEgi01 Awaiting requested review from DaEgi01

Assignees

@originalfoo originalfoo

Labels
code cleanup Refactor code, remove old code, improve maintainability
Projects
None yet
Milestone
11.6.5.2
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@originalfoo @kianzarrin @krzychu124