Conversation
First implementation of Ordered Map and Ordered Set
Small changes to address my mistakes in naming.
First implementation of queues based on implementation of Hood Melville queues from "Purely Functional Data Structures" Chris Okasaki.
Little correction of definitions.
Foxinio
left a comment
Foxinio
left a comment
There was a problem hiding this comment.
I wrote some comments, suggestions made in them apply in more places than specified, and i feel like some of them i already pointed out in my previous review.
Foxinio
left a comment
Foxinio
left a comment
There was a problem hiding this comment.
I pointed out some needed of changes, couple of them need to be applied across whole file, or commit (mainly fixing spacing, indentation, and formatting). Also please add documentation comments to all public facing constructs, since in many instances it isn't clear what functions do without diving into implementation
Foxinio
left a comment
Foxinio
left a comment
There was a problem hiding this comment.
Generally this is headed in a good direction, but still needs some changes.
dominik-muc
left a comment
dominik-muc
left a comment
There was a problem hiding this comment.
From what I've seen and tested it works OK, however it could use some minor fixes. That being said, I think it provides a lot of functionality and we should look forward into merging. Improvements can be made later on, as they are not crucial, especially given that size of this PR is already huge.
Final touches before merge
wojpok
left a comment
wojpok
left a comment
There was a problem hiding this comment.
Core functions of HMQueue are copied from Okasaki's book, I believe there are no issues there. I recommend to delete map and foldLeft functions and replace them with combinations of toList, fromList.
Correctness of Maps and Sets hinges on the implementation of Red Black trees. The interaface of RB Trees is too robust to be read easily,. Since there are no comments that clarify any of the endless pattern matching cases, I can't do more that just believe that this implementation is ok. I would like to see some clarification about those trees.
Beside minor issues I have pointed out, this is more or less ready to be merged
| | Appending of NotNegativeInt, List Val, List Val | ||
| | Done of List Val | ||
|
|
||
| data HoodMelvilleQueue Val = |
There was a problem hiding this comment.
This type should be abstract. As is right now, it can't be used to annotate stuff outside this module. Also, name could be shorter, maybe HMQueue or just plain and simple Queue?
| ) | ||
| end | ||
|
|
||
| pub let emptyQueue = HMQueue Zero [] Idle Zero [] |
| pub method isEmpty self = isEmpty self | ||
| pub method snoc self = snoc self | ||
| pub method tail self = tail self | ||
| pub method push self = snoc self |
There was a problem hiding this comment.
Remane this to cons. Push is confusing.
lib/Map.fram
Outdated
|
|
||
| {## @brief Method that returns list of keys | ||
| ##} | ||
| , method domain : {type Val} -> T Val ->[] List Key |
There was a problem hiding this comment.
Why not toKeyList? This naming is confusing
lib/Map.fram
Outdated
| upperBoundLeqErr compare tree key | ||
|
|
||
| pub let make {Key} (compare : Key -> Key ->[] Ordered) = Map { | ||
| T = MapT Key |
lib/Set.fram
Outdated
|
|
||
| pub let make {Val} (compare : Val -> Val ->[] Ordered) = Set { | ||
| T = Tree Val | ||
| , empty = empty |
… other things to understand what is going on in RedBlackTree
There was a problem hiding this comment.
Pull Request Overview
This PR implements Ordered Map and Ordered Set data structures using a Red-Black Tree implementation, along with a Queue data structure based on the Hood-Melville algorithm.
Key Changes:
- Added Red-Black Tree implementation as the underlying data structure for ordered collections
- Implemented Hood-Melville Queue with lazy rotation state management
- Added
Orderedtype to Base/Types for comparison operations
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/RedBlackTree.fram | Core red-black tree implementation with insert, delete, search, split and join operations |
| lib/Queue.fram | Hood-Melville Queue implementation with amortized O(1) operations |
| test/stdlib/stdlib0005_Queue.fram | Test suite for Queue operations including push, pop, head, isEmpty, and list conversions |
| lib/Prelude.fram | Added trailing newline for consistency |
| lib/Base/Types.fram | Added Ordered data type for comparison results (Lt, Eq, Gt) |
Comments suppressed due to low confidence (1)
lib/Queue.fram:1
- Missing space after '::' operator. Should be 'value :: zipper' for consistency with similar patterns throughout the codebase (e.g., lines 197, 199, 208).
{# This file is part of DBL, released under MIT license.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| data HoodMelvilleQueue Val = | ||
| | HMQueue of NotNegativeInt, List Val, RotationState Val, | ||
| NotNegativeInt, List Val |
There was a problem hiding this comment.
Inconsistent indentation: line 29 should align with the opening parameter on line 28. The second line of the constructor parameters uses 1 space instead of 3 spaces for proper alignment.
| NotNegativeInt, List Val | |
| NotNegativeInt, List Val |
There was a problem hiding this comment.
Thanks for cutting code, it is much easier to manage then is was before.
First and foremost, some issues from Queue module are still unresolved. Please, put queues into separate MR. They are almost ready to be merged, we don't have to wait for Maps and Sets.
Secondly, I have pointed it out on many occasions and I will hold to that. This implementation of RedBlackTrees is really robust and exposes way too much control over this structure. Zippers shouldn't be at any point a part of public interface. Some interface changes are required. You can go 2 ways from here:
You can make this interface user friendly by defining some sort of abstract iterator that will enable in place modifications. The zipper will private within this context. Or you simply make RB Tree completely private and implement really basic Map data structure. It's up to you
|
I have an idea. Please begin with documenting the RBTree library. Explain each public function (its use case, assumptions, exceptions). Maybe the right documentation is everything this modile actually needs? |
6 participants
First implementation of Ordered Map and Ordered Set