Skip to content

[Feature] Implement Buffer.alloc and Buffer.allocUnsafe, add tests #4

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

Merged
merged 4 commits into from
Jul 18, 2019

Conversation

jtenner
Copy link
Contributor

@jtenner jtenner commented Jul 18, 2019

Notes:

  • Using toThrow() is nice here because we can assert the module will panic (and be caught inside as-pect)
  • Downside: throw does not exist in assemblyscript and it checks for unreachable() instead (we can't determine why the unreachable() occurred)
afterAll(() => {
   expect<i32>(RTrace.count()).toBe(value);
});
  • Great reuse of E_INVALIDLENGTH here

Thoughts?

@jtenner jtenner added the enhancement New feature or request label Jul 18, 2019
@jtenner jtenner requested a review from dcodeIO July 18, 2019 14:53
@jtenner jtenner self-assigned this Jul 18, 2019
@jtenner
Copy link
Contributor Author

jtenner commented Jul 18, 2019

I like this solution better. Count the references after each test. For some reason #allocUnsafe has 12 instead of 14, but it's nice to set a fixture like this.

@dcodeIO
Copy link
Member

dcodeIO commented Jul 18, 2019

Not sure what to do with the throw asserts, since everything that can be done at this point is suboptimal. Suggesting to ignore these things for now, because given the complexity this adds, asserting that an u32 comparison for example works doesn't exactly justify the effort.

A comment to improve this eventually, once exception handling becomes a thing, is great, of course.

@jtenner
Copy link
Contributor Author

jtenner commented Jul 18, 2019

Not sure what to do with the throw asserts, since everything that can be done at this point is suboptimal.

I don't know exactly what you mean by "suboptimal."

If you held my feet to a fire, I wouldn't mind leaving out the throw asserts. It does seem to be good enough for the std lib. There is also another part of me remembers the many bug fixes I had to make in jest-canvas-mock because I didn't follow the specification correctly and didn't throw errors where they needed to be thrown.

As for the RTrace stuff, we can definitely assume that AssemblyScript does the job correctly and remove those too.

@dcodeIO
Copy link
Member

dcodeIO commented Jul 18, 2019

With suboptimal I meant that all things throw in AS don't work properly anyway yet, thus requiring workarounds, respectively that baking reference counts into the tests is susceptible to breakage if something on either the compiler or optimizer side changes. Seems hard to maintain.

@jtenner
Copy link
Contributor Author

jtenner commented Jul 18, 2019

Yeah. The toThrow() assertion merely looks for unreachable(). I know it's suboptimal, but as-pect will support try/catch the moment AssemblyScript does, and I see no reason why we should avoid using it, despite being not optimal.

We should definitely remove the RTrace stuff.

@jtenner
Copy link
Contributor Author

jtenner commented Jul 18, 2019

Okay. I think the functions are ready for review. Anything that needs to be changed?

@jtenner jtenner merged commit c3a162d into master Jul 18, 2019
@dcodeIO dcodeIO deleted the buffer-alloc branch June 1, 2021 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants