1201421 - Rename TokenStream modifier and modifier exception to something more clear.

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Derived from bug 1195578.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from bug 1195578 comment #7)
> This is total bikeshed, but what would you think about renaming "Operand" to
> "Expression", and maybe "None" to "Operator"?  I read "operand" as too much
> like "operator", and I end up confusing myself regularly reading this stuff.
> Separate bug if you agree.

(In reply to Tooru Fujisawa [:arai] from bug 1195578 comment #9)
> About Modifier's name, "Expression" sounds like it contains leading unary
> "Operator", so that won't describe well.  I think we should use the name
> that describes what actually happens, so, the difference between RegExp vs
> Div.  In ES6 spec, following 4 symbols are used [1]:
> 
>  * InputElementDiv
>  * InputElementTemplateTail
>  * InputElementRegExp
>  * InputElementRegExpOrTemplateTail
> 
> I guess we could just use InputElementDiv (= None) and InputElementRegExp (=
> Operand).  InputElementTemplateTail is a bit different because we tokenize
> leading '}' separatedly, but we could keep using TemplateTail, as it's
> already there.  InputElementRegExpOrTemplateTail is not used as
> InputElementTemplateTail is different and RegExp cannot be there.  Not sure
> whether it's better to leave KeywordIsName there or move it to another
> place.

I'll draft post patch.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Change Modifier and ModifierException to InputElement and InputElementException in TokenStream.

Here's draft patch.

Changed Modifier enum to InputElement enum class, and ModifierException enum to InputElementException enum class.  Currently KeywordIsName stays same.

 before                            | after
-----------------------------------+------------------------------------------
Modifier enum                      | InputElement enum class
  TokenStream::None                |   InputElement::Div
  TokenStream::Operand             |   InputElement::RegExp
  TokenStream::KeywordIsName       |   InputElement::KeywordIsName
  TokenStream::TemplateTail        |   InputElement::TemplateTail
ModifierException enum             | InputElementException enum class
  TokenStream::NoException         |   InputElementException::None
  TokenStream::NoneIsOperand       |   InputElementException::DivIsRegExp
  TokenStream::OperandIsNone       |   InputElementException::RegExpIsDiv
  TokenStream::NoneIsKeywordIsName |   InputElementException::DivIsKeywordIsName

So, at least this would almost match to the spec.  One concern is that this will conflict with many people's patches :P

Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88a68a02fbee
  (also including patches in bug 1195578 and bug 1196041)

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8656506 [details] [diff] [review]
Change Modifier and ModifierException to InputElement and InputElementException in TokenStream.

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

I'm kind of meh on the name, even if it's the spec name.  The spec's tokenization language seems very ad-hoc, handwavy to me -- enough so, that the spec terms aren't something that immediately would trigger recognition in the mind of a savvy reader.  But the more I stare at it, the more it maybe starts to grow on me.  :-\  Bleh.  But the renaming at least is less illegible than the operand/operator, so go ahead with this even if I'm kinda on the fence.

Comment 3

[Jeff Walden [:Waldo]]

[2017-01-12 11:32:07] <Waldo> arai: as far as naming goes, XIsY I've found hard to remember what the sense is -- is it that X is now okay where Y was previously specified, or vice versa? -- and maybe AllowXAsY would be better/clearer naming
[2017-01-12 11:33:59] <arai> Waldo: yeah, the current name isn't nice
[2017-01-12 11:34:22] <arai> I agree AllowXAsY is better :)
[2017-01-12 11:34:27] <Waldo> I feel like I didn't have a problem with XIsY before, but for whatever reason it's just not reading right for me now

Bug 1319416's landing means that, at least as far as modifier exceptions for '/' within expressions goes -- that is, the current OperandIsNone -- spec-ful names are pretty easy to slot in, and they'd map well to the sense implied by the code.

(This is much less true for NoneIsOperand, whose interaction with the relevant grammar productions isn't so cut-and-dried, but rather is [in at least some cases] a result of hairy-reading lookahead restrictions.)