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.

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

Fix: Support self application of Buffer.append #165

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

Conversation

matthewhammer
Copy link
Contributor

Appending a buffer with itself causes divergence, which may surprise or disappoint some developers.

Possible fixes:

  1. Keep same behavior (diverge)
  2. Check for this case and trap, failing fast, with an assertion failure within Buffer.append
  3. Iterators keep their own state about when to stop, copying it from the Buffer's internal variables (same outcome as first cloning arg before using it for appending, but more efficient).

This PR uses solution 3.

Rationale: The current "problem" is that there's a read-write dependency between the buffer's size when appearing as an argument, and when (also) appearing as the receiver of the append invocation. To permit this call with the expected functional behavior, we just need to break this dependency (copying the number of buffer elements to read before increasing this variable). This PR does that with a minor remedy to the code.

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.

@@ -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?

@ggreif ggreif force-pushed the master branch 2 times, most recently from d52aecd to 08507fc Compare October 21, 2022 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants