1478045 - Replace ungetCodePointIgnoreEOL/ungetNonAsciiNormalizedCodePoint with SourceUnits::{peek,consumeKnown}CodePoint

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jeff Walden [:Waldo]]

I added these interfaces earlier, but upon further review they're not really the right thing.  Most cases where they're used, it's super-obnoxious to have to unget-and-undo-EOL-updating.  It's also messy to have to unget a variable number of code units -- a variable count that will be a bit more complicated in a UTF-8 world (and will involve us computing length-counts two separate places).  Much better to peek at the next code point -- the actual operation we usually want -- and act based on that, sometimes never getting the code point, sometimes consuming it based on that already-computed info.  It's maybe a bit subtle, but it's clearer than the alternative.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Implement SourceUnits::{peek,consumeKnown}CodePoint for cases where a code point must be gotten, tested against one or more predicates, then sometimes ungotten based on those predicates

The first patch adds these functions and introduces one use of them -- incidentally, the most complicated use, because it must manually address updates for EOL (no other users will).  Then I have patches replacing the other users of the existing functions, then a final patch to remove the old functions.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Use SourceUnits::peekCodePoint to look for an IdentifierStart after most decimal numeric literals, rather than getting and ungetting

Oh -- in the previous patch, note that PeekedCodePoint being CharT-parametrized is only to be able to assert proper length-in-units.  I assume compilers can boil away the multiple class instantiations in practice.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Use SourceUnits::peekCodePoint when examining a non-ASCII code point that might terminate a directive in a comment

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Use SourceUnits::peekCodePoint when examining a non-ASCII code point that might terminate an IdentifierName

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Use SourceUnits::peekCodePoint when examining a non-ASCII code point after non-decimal numeric literals and a subset of decimal numeric literals

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Remove TokenStreamChars::{ungetCodePointIgnoreEOL,ungetNonAsciiNormalizedCodePoint} as unused now that SourceUnits::{peek,consumeKnown}CodePoint have replaced them

I suppose I should note that the existing users are all known-non-ASCII, but {peek,consumeKnown}CodePoint handle ASCII as well as non-ASCII.  In principle I could exclude the ASCII case, maybe, but I'm not sure it buys anything, and I kinda feel like handling everything may come in handy if we use this peek-code-point functionality in any other places in the future.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Implement SourceUnits::{peek,consumeKnown}CodePoint, but with the peekCodePoint algorithm abstracted out into a helper function

Turns out this algorithm comes in handy for SourceUnits::findWindowEnd, so it makes sense to define it such that SourceUnits::peekCodePoint *and* that function can use it.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: One last bit of tweaking to this, for realz

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8994704 [details] [diff] [review]
One last bit of tweaking to this, for realz

Review of attachment 8994704 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/TokenStream.h
@@ +986,5 @@
> +template<>
> +struct SourceUnitTraits<mozilla::Utf8Unit>
> +{
> +  public:
> +    static constexpr uint8_t maxUnitsLength = 2;

4

@@ +993,5 @@
> +        return codePoint < 0x80
> +               ? 1
> +               : codePoint < 0x800
> +               ? 2
> +               : codePoint < 0x10000

might be nice to add constants for those boundaries in Unicode.h or somewhere, maybe in followup bug/patch.

@@ +1016,5 @@
> +        MOZ_ASSERT(lengthInUnits != 0, "bad code point length");
> +        MOZ_ASSERT(lengthInUnits == SourceUnitTraits::lengthInUnits(codePoint));
> +    }
> +
> +    static PeekedCodePoint none() {

can you add some comment at the top of this class that clarifies this class represents either a single code point or "none", and what the "none" actually means?
(basically similar thing as the comment for `SourceUnits::peekCodePoint()`)
without that it's surprising to see this method here.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a63f805c03
Implement SourceUnits::{peek,consumeKnown}CodePoint for the uncommon cases where a code point must be gotten, tested against one or more predicates, then sometimes ungotten based on those predicates.  (This is unfortunately a bit subtle, but getting and ungetting is arguably worse, because ungetting has to unget a variable number of code units -- whereas peeking can compute that number of code units and then use it directly when the peeked code point is consumed, avoiding double-computation and increased potential for error.)  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c79e79e350bf
Use SourceUnits::peekCodePoint to look for an IdentifierStart after most decimal numeric literals, rather than getting and ungetting.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/51e2ba3c67d6
Use SourceUnits::peekCodePoint when examining a non-ASCII code point that might terminate a directive in a comment.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/466d4573b39a
Use SourceUnits::peekCodePoint when examining a non-ASCII code point that might terminate an IdentifierName.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1d2c7b998ef
Use SourceUnits::peekCodePoint when examining a non-ASCII code point after non-decimal numeric literals and a subset of decimal numeric literals.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/d93c6f631ca7
Remove TokenStreamChars::{ungetCodePointIgnoreEOL,ungetNonAsciiNormalizedCodePoint} as unused now that SourceUnits::{peek,consumeKnown}CodePoint have replaced them.  r=arai

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b4a63f805c03
https://hg.mozilla.org/mozilla-central/rev/c79e79e350bf
https://hg.mozilla.org/mozilla-central/rev/51e2ba3c67d6
https://hg.mozilla.org/mozilla-central/rev/466d4573b39a
https://hg.mozilla.org/mozilla-central/rev/d1d2c7b998ef
https://hg.mozilla.org/mozilla-central/rev/d93c6f631ca7