-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Normative: add base64 and hex methods for Uint8Array #3655
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
Conversation
I added type modeling for each separate case of |
fe0c534
to
6c74789
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.
This looks great to me.
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.
Nothing with normative consequences, but lots of little editorial things to fix up. I'll take another look after the current comments are addressed.
Comments addressed, thanks @michaelficarra. |
spec.html
Outdated
<h1> | ||
DecodeBase64Chunk ( | ||
_chunk_: a String, | ||
_throwOnExtraBits_: a Boolean or ~not-applicable~, |
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.
I think Michael's suggestion for _throwOnExtraBits_
has made this parameter pretty convoluted. I was fine with the previous optional bool, or if Michael remains feeling strongly about that being an "abuse", then my preference is something like optional _optionalBehavior_: ~throw-on-extra-bits~
instead of the current state.
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.
My preference would be to put it back how it was.
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.
I really don't want to use optionality for anything other than defaulting to something you could have passed. If we don't like bool+1, what about a 3-state enum?
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.
There are two states that matter, the 3rd one is an artifact. Reflecting that state is more confusing than helpful to the reader, because they follow it all the way to realize it doesn't affect behavior.
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.
What if we just have a boolean there, and have a separate DecodeBase64Octet
AO that does:
DecodeBase64Octet(_chunk_: a String with length 4)
1. Return ! DecodeBase64Chunk(_chunk_, *true*).
2. NOTE: The above step does not throw because a String of length 4 never has extra bits
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.
Well octet isn't the right word, but probably there is a word for 24 bits. Or DecodeBase64CompleteChunk
EDIT: DecodeBase64Bidozen!
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.
"bidozen" could be both 6 or 24 tho. I think how it was is fine.
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.
Started doing this but ended up going a slightly different direction in f8b8442, now we have
DecodeFinalBase64Chunk (
_chunk_: a String of length 2 or 3,
_throwOnExtraBits_: a Boolean,
): either a normal completion containing a List of byte values, or a throw completion
and
DecodeFullLengthBase64Chunk (
_chunk_: a String of length 4
): a List of byte values of length 3
thoughts?
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.
WFM
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.
lgtm!
spec.html
Outdated
_alphabet_: *"base64"* or *"base64url"*, | ||
_lastChunkHandling_: *"loose"*, *"strict"*, or *"stop-before-partial"*, | ||
optional _maxLength_: a non-negative integer, | ||
): a Record with fields [[Read]] (an integral Number), [[Bytes]] (a List of byte values), and [[Error]] (either ~none~ or a *SyntaxError* object) |
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.
): a Record with fields [[Read]] (an integral Number), [[Bytes]] (a List of byte values), and [[Error]] (either ~none~ or a *SyntaxError* object) | |
): a Record with fields [[Read]] (an integer), [[Bytes]] (a List of byte values), and [[Error]] (a *SyntaxError* object or ~none~) |
- Other appearances of
[[Read]]
indicate that it's an integer, not an integral Number. - In a type disjunction, multi-valued types should precede singular values.
- "either" is unnecessary after a left paren.
Additionally, this Record type should maybe be named and declared separately.
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, fixed. I don't personally think it's worth naming this type so I've left it unnamed for now.
spec.html
Outdated
</dl> | ||
<p>The <dfn id="standard-base64-alphabet">standard base64 alphabet</dfn> is *"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"*, i.e., the String whose elements are the code units corresponding to every letter and number in the Unicode Basic Latin block along with *"+"* and *"/"*.</p> | ||
<emu-alg> | ||
1. Let _byteSequence_ be the unique sequence of 3 bytes resulting from decoding _chunk_ as base64 (such that applying the base64 encoding specified in section 4 of <a href="https://datatracker.ietf.org/doc/html/rfc4648">RFC 4648</a> to _byteSequence_ would produce _chunk_). |
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.
Normally, the spec uses "such that" to express a constraint, but that's presumably not what's intended here. Maybe:
1. Let _byteSequence_ be the unique sequence of 3 bytes resulting from decoding _chunk_ as base64 (such that applying the base64 encoding specified in section 4 of <a href="https://datatracker.ietf.org/doc/html/rfc4648">RFC 4648</a> to _byteSequence_ would produce _chunk_). | |
1. Let _byteSequence_ be the unique sequence of 3 bytes resulting from decoding _chunk_ as base64. (That is, applying the base64 encoding specified in section 4 of <a href="https://datatracker.ietf.org/doc/html/rfc4648">RFC 4648</a> to _byteSequence_ would produce _chunk_.) |
I also tried the parenthetical as a NOTE step and as an Assert step. They work, but they don't have the same feel.
Alternatively, don't get into "decode" at all:
1. Let _byteSequence_ be the unique sequence of 3 bytes resulting from decoding _chunk_ as base64 (such that applying the base64 encoding specified in section 4 of <a href="https://datatracker.ietf.org/doc/html/rfc4648">RFC 4648</a> to _byteSequence_ would produce _chunk_). | |
1. Let _byteSequence_ be the unique sequence of 3 bytes such that applying the base64 encoding specified in section 4 of <a href="https://datatracker.ietf.org/doc/html/rfc4648">RFC 4648</a> to _byteSequence_ would produce _chunk_. |
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.
I went with "i.e., the sequence such that", which makes it clearly expressing a constraint but I think reads better than either of these suggestions. In particular I do want to continue to describe this as "decoding" because that is how everyone thinks of it, even though the RFC does not formally define a decoding algorithm.
The rendered spec for this PR is available at https://tc39.es/ecma262/pr/3655. |
33b58b7
to
3dfa316
Compare
changes addressed
Pending stage 4.