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

Fix: Support self application of Buffer.append #165

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix: Support self application of Buffer.append #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f24d64d
regression test
matthewhammer Aug 19, 2020
e0a0fc2
fix behavior
matthewhammer Aug 19, 2020
402e678
semis
matthewhammer Aug 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Buffer.mo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,9 @@ public class Buffer<X> (initCapacity : Nat) {
/// Returns an [Iter](Iter.html#type.Iter) over the elements of this buffer.
public func vals() : { next : () -> ?X } = object {
var pos = 0;
var valsCount = count;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let?

public func next() : ?X {
if (pos == count) { null } else {
if (pos == valsCount) { null } else {
let elem = ?elems[pos];
pos += 1;
elem
Expand Down
19 changes: 19 additions & 0 deletions test/bufTest.mo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import Prim "mo:prim";
import B "mo:base/Buffer";
import I "mo:base/Iter";
import O "mo:base/Option";
import Debug "mo:base/Debug";

// test repeated growing
let a = B.Buffer<Nat>(3);
Expand Down Expand Up @@ -92,3 +93,21 @@ func natIterEq(a:I.Iter<Nat>, b:I.Iter<Nat>) : Bool {
assert (c.toArray().size() == 2);
assert (c.toVarArray().size() == 2);
};

// regression test: self-append does not diverge
{
let c = B.Buffer<Nat>(0);
let d = B.Buffer<Nat>(0);

c.add(1); d.add(1);
c.add(2); d.add(2);
c.add(3); d.add(3);

Debug.print "append test 1: cloning avoids the issue";
d.append(d.clone());
Debug.print "append test 2: cloning not necessary";
c.append(c);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we want to allow this? The result is tied closely (maybe too close) to the evaluation order, e.g. what's the result of c.append(c.append(c)).append(c). The clone() approach makes this explicit, which is less likely to introduce bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so you dislike option 3. I can understand why.

So, do you want to choose options 1 or 2 (see my list)? Or add other design option?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think option 2 makes sense. They can always clone the buffer if they want to self append. I would expect append to be associative. Self append breaks that rule.

Debug.print "success";

// to do -- two buffers are equal
};