1428863 - Grab-bag of various token stream/parser changes to make those classes amenable to instantiation for single-byte characters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jeff Walden [:Waldo]]

I've had these patches since before the all-hands.  With them, and then with one additional +149,-29 patch that does known-stupid things just to make something compilable, I can instantiate a single-byte token stream and parser.  (Some of the extra implementation is stub |return false;| sorts of things -- this is just to prove it's far enough to compile, not to work.)

I'm not sure I have entirely coherent ideas how to describe the purposes of all these (except to the extent of "get us to the point to do that show-it's-possible" patch), so the bug description is a bit silly.  Not worth trying to come up with a better one.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Pass a PerHandlerParser<ParseHandler> to FoldConstants rather than a GeneralParser<ParseHandler, CharT>

This eliminates the need to duplicate constant-folding for one/two-byte parsing.  Obviously, such duplication would be very silly and mostly useless, as the little changes involved in doing this show.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Remove unused chars/length arguments from the ParserBase and PerHandlerParser constructors

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Change a few TokenStream::* uses in AsmJS.cpp to TokenStreamShared::*

TokenStream no longer has the meaning it's traditionally had -- rather than Parser containing a TokenStream, you have a TokenStreamAnyChars in ParserBase and a TokenStreamSpecific in GeneralParser.  TokenStream exists only for tokenizing/parsing regexps any more, and really that use is just silly and should go away.  (I don't know whether I'll do that soon or not.  Hopefully.)  So this stuff should use TokenStreamShared as sort of the closest thing to what TokenStream used to be, for this purpose.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Add non-qualified Position typenames to various parser structs

TokenStream's historic meaning is going away (see the prior patch).  Position is really |typename TokenStreamCharsBase<CharT>::Position|.  Obviously it's silly to type out *that*.  So really it makes sense to just |using| this into the Parser classes that really need it.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Adjust Parser::newRegExp to be roughly CharT-agnostic (up to a static_assert restricting it to char16_t, as a flag of a place requiring changes for UTF-8 handling)

Without this, obviously you get compile errors trying to assign to |const char16_t*| from a single-byte-derived thing.  The single-byte instantiation will look quite different (probably it'll be explode the string to UTF-16 and then handle that by an entirely separate code path...except then pointing at the location of an error will be wrong, so I'm not sure about that), so no reason for this function to be defined in a CharT-agnostic way, versus specializing.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Make TokenStreamChars have a delegated constructor

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Add TokenStreamChars::asSpecific to consolidate a downcast into a helper function

Comment 8

[Jeff Walden [:Waldo]]

Attachment: De-indent TokenStreamSpecific::getDirective a level

This arguably is a little a matter of taste, but it seems better to me this way.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Add and use TokenStreamChars::copyTokenbufTo

This is sort of weirdly inlined right now, and if we do UTF-8 single-byte handling, then we can't just do a literal element-by-element copy.  Better to split it out, then there's one thing to specialize when UTF-8 happens.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Minimize access to various base-class |using| declarations in TokenStreamSpecific

General access hygiene.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Move TokenStreamChars::atomizeChars into TokenStreamCharsBase -- no need for it in the more-derived class as long as it can be specialized for multiple character types

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 8940813 [details] [diff] [review]
Add non-qualified Position typenames to various parser structs

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

::: js/src/frontend/Parser.h
@@ +648,5 @@
>  class GeneralParser
>    : public PerHandlerParser<ParseHandler>
>  {
> +  public:
> +    using TokenStream = TokenStreamSpecific<CharT, ParserAnyCharsAccess<GeneralParser>>;

so, "TokenStream" identifier means different things between inside and outside GeneralParser, right?
it's a bit scary, but I'm fine as long as the another definition is renamed shortly.

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 8940824 [details] [diff] [review]
De-indent TokenStreamSpecific::getDirective a level

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

::: js/src/frontend/TokenStream.cpp
@@ +1133,5 @@
>  {
>      MOZ_ASSERT(directiveLength <= 18);
>      char16_t peeked[18];
>  
> +    if (!peekChars(directiveLength, peeked) || !CharsMatch(peeked, directive))

I prefer separating them into 2 if-s, since now they're just for early-return.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Move TokenStreamChars::copyTokenbufTo to TokenStreamCharsBase

Turns out I can introduce this a little bit higher up in the class hierarchy.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01ca5c5f3d0
Pass a PerHandlerParser<ParseHandler> to FoldConstants rather than a GeneralParser<ParseHandler, CharT>, eliminating the need to duplicate constant-folding for one/two-byte parsing.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a4be20ecde2
Remove unused chars/length arguments from the ParserBase and PerHandlerParser constructors.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/98378bbaf807
Change a few TokenStream::* uses in AsmJS.cpp to TokenStreamShared::*, because TokenStream no longer has the meaning it's traditionally had.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae260591e20e
Add non-qualified Position typenames to various parser structs.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/591f66dd6bcf
Adjust Parser::newRegExp to be roughly CharT-agnostic (up to a static_assert restricting it to char16_t, as a flag of a place requiring changes for UTF-8 handling).  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1406363fb17
Make TokenStreamChars have a delegated constructor.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/118c5618f348
Add TokenStreamChars::asSpecific to consolidate a downcast into a helper function.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/325b6c9e845e
De-indent TokenStreamSpecific::getDirective a level.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/6349fe7d8cbf
Add and use TokenStreamChars::copyTokenbufTo.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/515639ccfaf2
Minimize access to various base-class |using| declarations in TokenStreamSpecific.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/24b899860ad0
Move TokenStreamChars::atomizeChars into TokenStreamCharsBase -- no need for it in the more-derived class as long as it can be specialized for multiple character types.  r=arai

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d01ca5c5f3d0
https://hg.mozilla.org/mozilla-central/rev/2a4be20ecde2
https://hg.mozilla.org/mozilla-central/rev/98378bbaf807
https://hg.mozilla.org/mozilla-central/rev/ae260591e20e
https://hg.mozilla.org/mozilla-central/rev/591f66dd6bcf
https://hg.mozilla.org/mozilla-central/rev/c1406363fb17
https://hg.mozilla.org/mozilla-central/rev/118c5618f348
https://hg.mozilla.org/mozilla-central/rev/325b6c9e845e
https://hg.mozilla.org/mozilla-central/rev/6349fe7d8cbf
https://hg.mozilla.org/mozilla-central/rev/515639ccfaf2
https://hg.mozilla.org/mozilla-central/rev/24b899860ad0

Comment 17

[Jeff Walden [:Waldo]]

Actually, let's just keep this open for now.  I'll keep piling on small bits of patch that move us toward being able to instantiate the various TokenStream classes for Utf8Unit (bug 1426909, still WIP but the Utf8Unit bits seem solid) here until Utf8Unit (and a way to have UTF-8-containing strings from which a char* can be extracted when needed) has actually landed.

Comment 18

[Jan de Mooij [:jandem]]

Given the merge next week.. It might make sense to file a new bug for patches landing in 60 :)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eecb5e20e10
Move TokenStreamChars::copyTokenbufTo to TokenStreamCharsBase.  r=arai

Comment 20

[Jeff Walden [:Waldo]]

Oh, that's probably fair.  Will remove the keyword.

Comment 21

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/9eecb5e20e10