-
Notifications
You must be signed in to change notification settings - Fork 16
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
Implement v1 in terms of v2 #73
Conversation
This builds on top of #72. |
d02227f
to
494e46b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive stuff!
before, after = before[1:], after[1:] | ||
} | ||
} | ||
t.Errorf("known failures mismatch (-old +new):\n%s", strings.Join(diff, "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use e.g. go-cmp? I understand as a dependency it may be a problem for integrating into std, but I assume std will only care about the finished product as a single commit, or perhaps a few commits to land the changes in chunks, and not the full history - so we can import other deps in the meantime I assume.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I guess that's true. Given that I already wrote this diff, it's probably fine.
v1/stream.go
Outdated
case '[': | ||
return Delim('['), nil | ||
case ']': | ||
return Delim(']'), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't these four use return Delim(kind), nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delim('[')
is a constant, so the compiler can pre-allocate this.
Delim(k)
isn't a constant, so there's likely going to be a heap allocation at return to box this in an interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I keep forgetting this. Perhaps add a comment to that effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, interesting, https://go.dev/play/p/gIsvN2C0hmF doesn't allocate. It seems that for many releases, the Go compiler avoids allocating "small" integers in an interface as it's already pre-allocated values for them.
It seems that the entire range of possible values for a uint8
are pre-allocated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! That was @josharian some time ago :) https://go-review.googlesource.com/c/go/+/216401
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @josharian!
Remove all the core functionality of v1 and instead implement it in terms of v2 with the appropriate options specified. Many v1 test cases currently failure because the v1 emulation in v2 is not a sufficient reproduction of historial v1 behavior. The list of known failures are stored in v1/failing.txt, which allows such failing tests to be skipped. As the v1 emulation in v2 improves, the failing.txt list should eventually become empty.
Remove all the core functionality of v1
and instead implement it in terms of v2
with the appropriate options specified.
Many v1 test cases currently fail because the v1 emulation in v2
is not a sufficient reproduction of historial v1 behavior.
The list of known failures are stored in v1/failing.txt,
which allows such failing tests to be skipped.
As the v1 emulation in v2 improves, the failing.txt list
should eventually become empty.