Vector module (#258) by kulson · Pull Request #306 · fram-lang/dbl

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

This reverts commit 77d9937.

ppolesiuk

Overall, everything is going in the right direction. The interface is minimal, but it can be later extended. One thing must be fixed before merging: the public interface must not allow to read values created with dbl_magic. Other thing that might be rethought is when, and how the capacity should be changed. See comments below, for details.

ppolesiuk · 2026-02-24T13:42:29Z

lib/Testing.fram

    log' LT_Exit (msg.unwrapOr "Test terminated early.");
    testExit ()
-end
+end


New line character was removed from non changed file. First, we should avoid unnecessary changes like this, second, we use UNIX convention, where each nonempty text file ends with newline character.

ppolesiuk · 2026-02-24T13:45:04Z

lib/Vector.fram

+  let n = n ||| (n>>8) in
+  let n = n ||| (n>>16) in
+  let n = n ||| (n>>32) in
+  let n = n ||| (n>>64) in


For 64-bit (or even 63) integers this line changes nothing. It can be safely removed.

ppolesiuk · 2026-02-24T13:54:04Z

lib/Vector.fram

+pub let makeVector {T, ~mut : Mut E} (size : Int) =
+  let capacity = (getSmallestUpperPow2 size) in
+  Vector
+  { content    = ~mut.ref (~mut.makeArray capacity ((extern dbl_magic : Int -> T) 0))


The extern dbl_magic is used to create an element of arbitrary type. This would be correct unless the public interface allows reading such elements. For example,

let v = makeVector {T=String, ~mut=ioMut} 42 in v.get 0 + "A"

results in runtime error. To avoid such problems we should forbid creating non-empty vectors without providing the default value. Creating empty vectors is still ok.

ppolesiuk · 2026-02-24T13:55:58Z

lib/Vector.fram

+
+{## Converts a list into a vector. ##}
+pub let fromList {~mut : Mut E} (l : List T) =
+  let vec = makeVector {T=T} l.length in


This line could be slightly simplified.

Suggested change

let vec = makeVector {T=T} l.length in

let vec = makeVector {T} l.length in

This is not very important comment.

ppolesiuk · 2026-02-24T13:56:57Z

lib/Vector.fram

+  self.setSize 0;
+  self.setCapacity 0;
+  self.setContent (self.mut.makeArray 0 ((extern dbl_magic : Int -> T) 0))
+


This file ends with a space (without a new line).

ppolesiuk · 2026-02-24T13:57:42Z

test/stdlib/Vector.fram

+    let lst = vec.back in
+    assertEqS fst "abc";
+    assertEqS lst "a"))
+


File ends with a space (as above)

ppolesiuk · 2026-02-24T14:38:02Z

lib/Vector.fram

I don't see, why the capacity of a vector must be a power of 2 (except for the empty vector). Maybe it would be better to allow any capacity, and multiply it by 2 (or any other constant), when the vector must be resized. This would allow to convert lists to vectors without unnecessary memory overhead, and I thing would slightly simplify the code. Moreover, I don't see why the vector is never resized down.

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

kulson added 3 commits February 22, 2026 19:45

Add: Vector module (fram-lang#258)

77d9937

Revert "Add: Vector module (fram-lang#258)"

40abb96

This reverts commit 77d9937.

Add: Vector module (fram-lang#258)

3906ceb

kulson force-pushed the 258 branch from 94961df to 3906ceb Compare February 22, 2026 19:02

Add: Vector module (fram-lang#258)

0d8a228

ppolesiuk requested changes Feb 24, 2026

View reviewed changes

kulson added 2 commits March 7, 2026 11:56

Add: Vector module (fram-lang#258)

10501dd

Add: Vector module (fram-lang#258)

923f8a0

	let vec = makeVector {T=T} l.length in
	let vec = makeVector {T} l.length in

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!

ppolesiuk 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants