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 for #16 and a few bonus changes #17

Closed
wants to merge 14 commits into from
Closed

Fix for #16 and a few bonus changes #17

wants to merge 14 commits into from

Conversation

Mithgol
Copy link
Contributor

@Mithgol Mithgol commented May 27, 2012

  • Moved compatibility checks out of methods (fixes for the sake of speed, move compatibility checks out of your methods #16).
  • Used window[type + 'Array'] instead of all[type + 'Array'], otherwise several tests fail in Firefox 13 with all is not defined.
  • Restructured the first three tests to avoid the Expected at least one assertion, but none were run - call expect(0) to accept zero assertions message.
  • As MDN docs say, arguments.callee is expensive, and forbidden in ECMAScript 5 strict mode, and should be avoided in favour of the function's name unless it's anonymous. Having considered that, I used this instanceof jDataView instead of this instanceof arguments.callee in the constructor.

…he constructor becomes slightly faster (`if`s are passed once, not eight times)
… `littleEndian` argument (which is known only at runtime, not at the constructor)
@vjeux
Copy link
Member

vjeux commented Jun 7, 2012

I'm sorry for the delay but I've been really busy the last few weeks, I'm going to take a look at it as soon as I find some time :)

Thanks for working on it, that's awesome btw!

@Mithgol
Copy link
Contributor Author

Mithgol commented Jun 7, 2012

Thank you for your attention. I understand that this pull request is larger than the other two that you merged today and thus it will take more time to evaluate.

By the way, the 28a3ab9 commit brings here several lines of code from the already merged #19 to avoid a merge conflict in msysgit that I encountered today.

@Mithgol
Copy link
Contributor Author

Mithgol commented Jun 30, 2012

The e1c16be fix for #22 is flawed (does not pass the tests).

Do not merge yet.

@Mithgol
Copy link
Contributor Author

Mithgol commented Jun 30, 2012

I came to an understanding that the tests were written to expect little-endian defaults. That defaults, however, are removed in e1c16be.

You may merge this pull request immediately, just remember to rewrite the multi-byte tests and the jParser after that.

@Mithgol
Copy link
Contributor Author

Mithgol commented Jun 30, 2012

Of course, this should be done carefully.

I guess the jDataView's version has to be pulled — and then the new jParser made to require that new version.

@Mithgol
Copy link
Contributor Author

Mithgol commented Jun 30, 2012

Tests are done, and now jParser is the only thing that remains to be rewritten after #22 is fixed.

@vjeux
Copy link
Member

vjeux commented Jun 30, 2012

I'm going to sleep, I'll do that tomorrow :) Thanks a lot for the help on it

@Mithgol
Copy link
Contributor Author

Mithgol commented Jun 30, 2012

:)

@Mithgol
Copy link
Contributor Author

Mithgol commented Jun 30, 2012

jParser would be corrected by vjeux/jParser#1 and that fix seems backwards-compatible (contrary to what I feared).

Thus the version bump to 1.1.0 here does not seem necessary for jParser, but may still be handy for other modules out there.

if (jDV._isArrayBuffer && (jDV._start + byteOffset) % size === 0 && (size === 1 || littleEndian)) {
// ArrayBuffer: we use a typed array of size 1 if the alignment is good
// ArrayBuffer does not support endianess flag (for size > 1)
return new window[type + 'Array'](this.buffer, this._start + byteOffset, 1)[0];
Copy link
Member

Choose a reason for hiding this comment

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

if we are in node and use an ArrayBuffer, it's going to crash as window does not exist.

@Mithgol
Copy link
Contributor Author

Mithgol commented Jul 1, 2012

Fixed, thanks.

@Mithgol
Copy link
Contributor Author

Mithgol commented Aug 8, 2012

After #16 and #23 is closed, the only real value of this commit is that it contains a fix for #22, but a pull request for that should be separate.

Closing.

@Mithgol Mithgol closed this Aug 8, 2012
@Mithgol
Copy link
Contributor Author

Mithgol commented Aug 8, 2012

That new (separate) pull request is #24.

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.

for the sake of speed, move compatibility checks out of your methods
2 participants