Vector module (#258) #306

kulson · 2026-01-10T17:06:04Z

Vector module implementation with tests

wojpok

Overall this PR is a nice addition to standard library. Some cosmetic changes are required. As far as I am concerned the logic is correct, so there shouldn't be much work to do

wojpok · 2026-01-11T14:28:15Z

test/stdlib/Vector.fram

+
+let vec = makeVector {T=Int} 4
+let _ =
+  assert (vec.size == 4);


Please, use designated testing framework for testing. Use String tests as an example

wojpok · 2026-01-11T14:31:04Z

lib/Vector.fram

+parameter T
+
+# Helper methods for vector implementation.
+method getContentAt (Vector{ content } : Vector E T) (n : Int) =


Formatting is off here. We usually write Vector {content} or Vector { content }. This extra space before opening bracket is somewhat mandatory. This applies to entire file

wojpok · 2026-01-11T14:33:16Z

lib/Vector.fram

+  let n = n ||| (n>>64) in
+  n+1
+
+pub let makeVector {T} (size : Int) =


Documentation is missing. Please, add docstrings to all public functions and methods

wojpok · 2026-01-11T14:35:36Z

lib/Vector.fram

+#}
+pub method resize (self : Vector E T) (n : Int) =
+  self.setSize n;
+  let new_capacity = getSmallestUpperPow2 n in


Use camelCase for variables

wojpok · 2026-01-11T14:37:10Z

lib/Vector.fram

+
+# Get the nth element of a vector.
+pub method get (self : Vector E T) (n : Int) =
+  assert {msg="Vector is empty"} (self.empty.neg);


This assert is excessive. Second one is sufficient on its own

wojpok · 2026-01-11T14:43:56Z

lib/Vector.fram

+  self.setSize 0;
+  self.setCapacity 0;
+  self.setContent (self.mut.makeArray 0 (None : Option T))
+


This interface is pretty minimal. It would be nice to have al least iter and fold methods, as well as toList/fromList conversion functions.

kulson · 2026-01-12T18:50:04Z

This commit includes almost all the fixes you mentioned. At this point, I haven't implemented iter and fold methods yet.

wojpok

Code is much cleaner now. Upon more careful study I have found more issues with the code

wojpok · 2026-01-12T20:04:59Z

lib/Vector.fram

+parameter E
+parameter T
+
+{## Helper methods for vector implementation ##}


This is comment about internal implementation, it shouldn't be a part of a documentation. Single # would be enough. Also, please add ## ## Vector library at the top of the file. Title is missing

wojpok · 2026-01-12T20:06:31Z

lib/Vector.fram

+import open /Base/Bool
+import open /List
+
+data Vector E T = Vector of


Make this type abstract. It is visible outside the module, so users should be able to use this type to annotate stuff

wojpok · 2026-01-12T20:10:11Z

lib/Vector.fram

+  n+1
+
+{## Creates vector of given size (filled with Nones). ##}
+pub let makeVector {T} (size : Int) =


You have hardcoded ioMut handler, which disallows using vectors with other mutability handlers. Desired handler should be passed as an argument to this function.

wojpok · 2026-01-12T20:11:01Z

lib/Vector.fram

+pub method empty (Vector {size} : Vector E T) = size.get == 0
+
+{##
+Resizes the vector to contain n elements.


Content of multiline docstrings should be indented with 2 spaces

wojpok · 2026-01-12T20:19:04Z

test/stdlib/Vector.fram

+import open /Vector
+import open Testing
+
+let vec = makeVector {T=Int} 4


Tests needs rework. All actions should be performed inside testCase body. Since IO is hardcoded into vector, it is impossible to fix right now. Once you enable other handlers, rework those tests. You will need to make a second wrapper, that will provide mut handler. Tests will look like this:

testCase "test" (fn _ => hMut (fn mut => # test body ))

wojpok · 2026-01-12T20:22:41Z

lib/Vector.fram

+  size := n 
+
+{## Returns maximum size of vector. ##}
+pub let maxVectorSize = maxArrayLength


This is wrong. Vectors are always aligned to the nearest power of 2. Arrays can have any arbitrary size, so there might a case where maxVectorSize is not possible to allocate. For example if maxArrayLength is 17, then the vector of size 17 will require internal array of size 32, which is impossible to allocate. I suggest deleting this. I don't think there is any demand for contant such as this one

wojpok · 2026-01-12T20:26:58Z

test/stdlib/Vector.fram

+    assertEqS cap 4);
+
+  testCase "checks max vector size constant" (fn _ =>
+    assertEqS maxVectorSize maxArrayLength);


This shouldn't be tested at all

kulson · 2026-01-17T14:06:19Z

Tests are not working yet because the testing framework is still pending merge.

Xaro8

Overall looks fine to me. Agree that fold left/right may be useful, also maybe resize functionality should be tested (outside of push_back).

kulson · 2026-02-01T18:32:33Z

From now on the vector no longer stores Option T.
Instead it uses a dbl_magic, which means the value must be initialized before reading.

wojpok · 2026-02-03T09:08:57Z

I think, you were meant to use dbl_magic to convert number 0 to desired type T. The difference is subtle, but if we ever were to compile this code, zero (null) will not require any additional memory allocation. Also, please fix issues with last lines in files. Last line should be completely empty

Add: Vector module (fram-lang#258)

bf4a720

wojpok suggested changes Jan 11, 2026

View reviewed changes

Add: Vector module (fram-lang#258)

04c589f

wojpok suggested changes Jan 12, 2026

View reviewed changes

Add: Vector module (fram-lang#258)

51ef7c2

Xaro8 self-requested a review January 29, 2026 11:24

Xaro8 suggested changes Jan 29, 2026

View reviewed changes

kulson and others added 2 commits February 1, 2026 19:04

Merge branch 'fram-lang:master' into 258

b8b37ea

Add: Vector module (fram-lang#258)

06537ac

Vector module (#258) #306

Are you sure you want to change the base?

Vector module (#258) #306

Uh oh!

Conversation

kulson commented Jan 10, 2026

Uh oh!

wojpok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kulson commented Jan 12, 2026

Uh oh!

wojpok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kulson commented Jan 17, 2026

Uh oh!

Xaro8 left a comment

Choose a reason for hiding this comment

Uh oh!

kulson commented Feb 1, 2026

Uh oh!

wojpok commented Feb 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants