Skip to content

Define encodeInto() API#166

Merged
annevk merged 8 commits intomasterfrom
annevk/encodeInto
Jan 9, 2019
Merged

Define encodeInto() API#166
annevk merged 8 commits intomasterfrom
annevk/encodeInto

Conversation

@annevk
Copy link
Member

@annevk annevk commented Dec 13, 2018

This enables converting strings into byte sequences of pre-allocated buffers.

Also cleanup TextEncoder a bit.

Tests: ...

Fixes #69.


Preview | Diff

This enables converting strings into byte sequences of pre-allocated buffers.

Also cleanup TextEncoder a bit.

Tests: ...

Fixes #69.
@annevk
Copy link
Member Author

annevk commented Dec 13, 2018

The reason this does not contain the StringView dictionary is because it can be added later if there's indeed a need for it. This also does not require any particular destination length as either way out-of-band needs to be dealt with and it seems better to be flexible for simple string use cases that would otherwise have to write completely different code.

@hsivonen
Copy link
Member

Looks correct to me, but it took me a moment to realize that the algorithm always loops over all tokens and the loop just does not necessarily write all the output. While an early return might be just an optimization, it might make the algorithm more comprehensible.

@annevk
Copy link
Member Author

annevk commented Dec 13, 2018

Thanks for pointing that out, I think it's actually wrong. Say we had 3 bytes left and the next input required 4 bytes and the input after that 1 byte. This would skip the 4 bytes input and then write the 1 byte one.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Dec 13, 2018
Copy link
Collaborator

@ricea ricea left a comment

Choose a reason for hiding this comment

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

Generally looks good. There's a few areas where the wording needs tightening up, as you've noted.

@annevk
Copy link
Member Author

annevk commented Dec 13, 2018

Do you know what the wording for those places should be? IDL doesn't really define any language around IDL ArrayBuffer objects... I guess I can file an issue upstream at least.

Other things:

  • Add an example.
  • Clarify what read and written represent.
  • Maybe clarify why there's no decodeInto().

@inexorabletash
Copy link
Member

inexorabletash commented Dec 13, 2018

I think this is implied by your last comment, but just to confirm: as written, destination's view bounds (offset, length) onto its ArrayBuffer is seemingly ignored by the current algorithm. i.e. in this case:

const ab = new ArrayBuffer(5);
const ua = new Uint8Array(ab, 1, 2);
new TextEncoder().encodeInto("ABCDE", ua);

... I would expect ab to end up as [0, 65, 66, 0, 0].

(I don't see test cases for this either)

@annevk
Copy link
Member Author

annevk commented Dec 13, 2018

Yeah, that clearly needs to be worded better. (I added tests.)

@annevk
Copy link
Member Author

annevk commented Dec 14, 2018

I think this is good now apart from the example which I'd like feedback on over at #69 (comment). (And there's a Bikeshed issue I'll resolve with Tab.)

@annevk annevk added addition/proposal New features or enhancements topic: api labels Dec 14, 2018
foolip pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 19, 2018
@annevk
Copy link
Member Author

annevk commented Dec 21, 2018

I haven't explained why there's no decodeInto(). I suppose "bytes in encoding X" -> "bytes in UTF-8" could make sense, but let's only add that if there's a request.

Anyone want to do a final review pass? (Tests need one too.)

@annevk
Copy link
Member Author

annevk commented Jan 8, 2019

I'll land this and the tests tomorrow, unless there are further comments.

@annevk annevk added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label Jan 9, 2019
@annevk
Copy link
Member Author

annevk commented Jan 9, 2019

cc @whatwg/documentation

annevk added a commit to web-platform-tests/wpt that referenced this pull request Jan 9, 2019
@annevk
Copy link
Member Author

annevk commented Jan 9, 2019

@annevk annevk merged commit 9d75583 into master Jan 9, 2019
@annevk annevk deleted the annevk/encodeInto branch January 9, 2019 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation topic: api

Development

Successfully merging this pull request may close these issues.

4 participants