core: store blocks as doubly-linked list on Region [2/2] #2795
Conversation
superlopuh
requested review from
AntonLydike,
georgebisbas,
math-fehr,
PapyChacal and
webmiche
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2795 +/- ##
========================================
Coverage 89.80% 89.81%
========================================
Files 396 396
Lines 48836 49010 +174
Branches 7480 7503 +23
========================================
+ Hits 43857 44018 +161
- Misses 3796 3803 +7
- Partials 1183 1189 +6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Makes sense to me, though I think we really should abstract away the double linked list.
I'm pretty sure we can abstract a DoubleLinkedList[T] class that will contain almost all of the logic, and then with a bit of inheritance we can get back what you have (in both blocks and operations).
Otherwise, I'm fine merging it like it is
| @@ -283,46 +283,34 @@ def insert_op_before(op: Operation, new_op: Operation): | |||
| def move_region_contents_to_new_regions(region: Region) -> Region: | |||
| """Move the region blocks to a new region.""" | |||
| new_region = Region() | |||
| for block in region.blocks: | |||
|
It would be nice to have such an abstraction, but I would say that this is something we want to hide from users, at least the mutability aspect. Let's chat about this at some point. |
|
|
||
| def __len__(self): | ||
| return len(self._blocks) | ||
| i = 0 | ||
| for _ in self: |
There was a problem hiding this comment.
as opposed to enumerating or while (next := next.next) is not None?
This migrates the blocks to be a doubly linked list.