Closed Bug 1336783 Opened 8 years ago Closed 8 years ago

Remove TokenStream::KeywordIsName

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(3 files, 1 obsolete file)

Related to bug 1319416. TokenStream::KeywordIsName does totally different thing than other modifier, and it seems to make it hard to fix bug 1319416. from some experiments, checking keyword/name could be deferred to Parser::{labelOrIdentifierReference,bindingIdentifier}, by following way: 1. tokenize every special identifier as dedicated TokenKind (unless it has unicode escape) 2. when parser need possible Identifier / IdentifierName TokenKind, compare the given TokenKind with range that represents possible Identifier or IdentifierName, instead of comparing with TOK_NAME/TOK_YIELD directly 3. In Parser::{labelOrIdentifierReference,bindingIdentifier}, check PropertyName* of the token, to check unescaped name with this way, we don't have to check keyword (actually Reserved Word) in TokenStream, and also it have some merits that: a. TOK_NAME/TOK_YIELD handling can be merged b. we can use dedicated TokenKind for "as", "async", "from", etc, as contextual keyword, and parser logic becomes simpler c. Parser doesn't have to check escapeness at several places I'll post patch after extra try run.
(In reply to Tooru Fujisawa [:arai] from comment #0) > from some experiments, checking keyword/name could be deferred to > Parser::{labelOrIdentifierReference,bindingIdentifier} and also some more places that checks identifier, like * Parser::checkStrictBinding * ParserBase::isValidStrictBinding
here's draft patch that removes TokenStream::KeywordIsName. can I get your feedback, related to bug 1319416 fix? (it touches many places and I guess it will conflict with your patch there...) the details of the change are following: [reserved words check] * Moved the common part of Parser::{labelOrIdentifierReference,bindingIdentifier} to Parser::checkLabelOrIdentifierReference, that also checks Reserved Word * Changed Parser::checkStrictBinding to report with more appropriate error message * Added the following functions to check TokenKind of PropertyName*: * IsFutureReservedWord * IsStrictReservedWord * IsReservedWordLiteral to use in Parser::checkLabelOrIdentifierReference * Removed special handling for TOK_NAME with yield/await/let/static name * Stop checking reserved word error in TokenStream::getTokenInternal, and removed the following methods: * TokenStream::checkForKeyword * Moved awaitIsKeyword handling from TokenStream to Parser * Removed TokenStream::KeywordIsName and TokenStream::NoneIsKeywordIsName [contextual keyword] * Added the following contextual keyword TokenKind: * TOK_AS * TOK_ASYNC * TOK_EACH * TOK_FROM * TOK_GET * TOK_LET * TOK_OF * TOK_SET * TOK_STATIC * TOK_TARGET * Used above contextual keyword TokenKind in Parser, and removed the following methods: * Parser::checkUnescapedName * TokenStream::matchContextualKeyword [reserved words] * Added the following future reserved words/strict reserved words TokenKind to make them easy to handle in error reporting: * TOK_ENUM * TOK_IMPLEMENTS * TOK_INTERFACE * TOK_PACKAGE * TOK_PRIVATE * TOK_PROTECTED * TOK_PUBLIC * Added the following functions to check TokenKind with range: * TokenKindIsKeyword * TokenKindIsContextualKeyword * TokenKindIsFutureReservedWord * TokenKindIsStrictReservedWord * TokenKindIsReservedWordLiteral * TokenKindIsReservedWord * TokenKindIsPossibleIdentifier * TokenKindIsPossibleIdentifierName * Added MUST_MATCH_TOKEN_COND_MOD/MUST_MATCH_TOKEN_COND macro to check arbitrary condition (TokenKindIs*) instead of comparing with single TokenKind * Use TokenKindIsPossibleIdentifier or TokenKindIsPossibleIdentifierName where we were checking TOK_NAME/TOK_YIELD in Parser * Changed TokenStream::{currentName,nextName} to return PropertyName for all TokenKindIsPossibleIdentifierName==true case, instead of only TOK_NAME/TOK_YIELD [filename] * Renamed the following files and also changed "keyword" to "reserved word": * js/src/vm/Keywords.h => js/src/vm/ReservedWords.h * js/src/jsautokw.h => js/src/jsautorw.h [other] * Fixed local ExportClause to check IdentifierReference
Attachment #8833732 - Flags: feedback?(jwalden+bmo)
Comment on attachment 8833732 [details] [diff] [review] Rework on reserved word and remove TokenStream::KeywordIsName. Review of attachment 8833732 [details] [diff] [review]: ----------------------------------------------------------------- This is far cleaner than it has any right to be. Never would have thought adding non-keyword reserved words to the master list would actually make things *cleaner*. I'm trying to land my two conflicting patches as fast as possible, but tree's closed right now. Will land and get out of the way of this ASAP. ::: js/src/frontend/Parser.cpp @@ +4647,5 @@ > > if (tt == TOK_RC) > break; > > + MUST_MATCH_TOKEN_COND(TokenKindIsPossibleIdentifierName(token), JSMSG_NO_IMPORT_NAME); I assume |token| doesn't actually compile here, right? And many other places. I'm guessing this is just f? purposes, mostly? @@ +4778,5 @@ > if (!importSpecSet) > return null(); > > + if (TokenKindIsPossibleIdentifierName(tt) || tt == TOK_LC || tt == TOK_MUL) { > + if (TokenKindIsPossibleIdentifierName(tt)) { TKIPI starts to get complex enough internally, that I'd like to avoid computing it multiple times -- and, computing it when it's not needed (i.e. when we have '{' or '*'). Please rearrange this whole |if| to if (tt == TOK_STRING) { ... } else { if (tt == TOK_LC || tt == TOK_MUL) { if (!namedImportsOrNamespaceImport(tt, importSpecSet)) return null(); } else if (TokenKindIsPossibleIdentifierName(tt)) { Node importName = newName(context->names().default_); ... } else { error(JSMSG_DECLARATION_AFTER_IMPORT); return null(); } if (!tokenStream.getToken(&tt)) return null(); if (tt != TOK_FROM) { error(JSMSG_FROM_AFTER_IMPORT_CLAUSE); return null(); } MUST_MATCH_TOKEN(TOK_STRING, JSMSG_MODULE_SPEC_AFTER_FROM); } @@ +4956,3 @@ > return null(); > if (foundAs) > + MUST_MATCH_TOKEN_COND(TokenKindIsPossibleIdentifierName(token), Needs braces. @@ +4995,5 @@ > // In that case let matchOrInsertSemicolonAfterNonExpression sort out > // ASI or any necessary error. > TokenKind tt; > if (!tokenStream.getToken(&tt, TokenStream::Operand)) > return null(); Could maybe use matchToken(TOK_FROM, TokenStream::Operand) now so you don't have to unget later. @@ +5012,5 @@ > if (!node || !pc->sc()->asModuleContext()->builder.processExportFrom(node)) > return null(); > > return node; > + } else { Don't need an |else| here. @@ +5018,5 @@ > + // should contain IdentifierReference. > + > + for (ParseNode* next = kid->pn_head; next; next = next->pn_next) { > + if (!next->isKind(PNK_EXPORT_SPEC)) > + continue; I must be reading this wrong somehow, because it looks like the only children appended to |kid| are *always* PNK_EXPORT_SPEC. So this loop never does anything but iterate everything and continue on each node. Why isn't this the case? But, I assume this loop fixes the additional test you added to export-declaration.js, so I'm maybe confused here. Assuming this loop does actually do anything, I don't see why identifier-checking can't be performed directly as the ExportClause is being parsed. Disillusion me? @@ +5121,3 @@ > return null(); > break; > case TOK_CLASS: Apropos of nothing, we really need to split up Parser::exportDeclaration into sub-functions to handle the various different possible syntaxes within it. The shared three-ish line tail can't possibly justify it. @@ +6767,3 @@ > // Otherwise a let declaration must have a name. > + if (TokenKindIsPossibleIdentifier(next)) { > + // "let" edge case deserves special comment. """ A "let" edge case deserves special comment. Consider this: """ Particularly, "'let' edge case" requires some sort of article -- a/an/the -- in front of it. Can't just lead with the noun phrase without one. (I can't articulate why.) @@ +6846,5 @@ > + // Avoid getting next token with None. > + if (tt == TOK_AWAIT && pc->isAsync()) > + return expressionStatement(yieldHandling); > + > + if (!TokenKindIsPossibleIdentifier(tt) && tt != TOK_LET) TokenKindIsPossibleIdentifier(TOK_LET) is true. So the second condition there is, if evaluated, always true. I think the prior system here of peeking, handling |let| if encountered and actually a declaration, then if necessary falling through to the label/expression cases, was the proper one. So some of this changing needs undoing. @@ +7056,5 @@ > + // Avoid getting next token with None. > + if (tt == TOK_AWAIT && pc->isAsync()) > + return expressionStatement(yieldHandling); > + > + if (!TokenKindIsPossibleIdentifier(tt) && tt != TOK_LET && tt != TOK_ASYNC) Again the latter two conditions seem vacuously true. And again, it seems to me that the previous scheme of handling specific cases, then falling through into the label/not-label cases, is the one we should keep using. @@ +7065,5 @@ > return null(); > > + if (tt == TOK_LET && nextTokenContinuesLetDeclaration(next, yieldHandling)) { > + return lexicalDeclaration(yieldHandling, /* isConst = */ false); > + } No braces. @@ +7498,5 @@ > > bool endsExpr; > > + // This is fast path for IdentifierReference. > + // Do not handle other possible identifier cases. Maybe "This only handles identifiers that *never* have special meaning anywhere in the language. Contextual keywords, keywords only reserved in strict mode, and other hard cases are handled outside this fast path." @@ +8584,5 @@ > + errorAt(offset, JSMSG_RESERVED_ID, ReservedWordToCharZ(ident)); > + return nullptr; > + } > + > + Extra blank line. @@ +8873,3 @@ > propAtom.set(tokenStream.currentName()); > // Do not look for accessor syntax on generators > + if (isGenerator || isAsync || !(ltok != TOK_GET || ltok != TOK_SET)) { Shouldn't that be !(ltok == TOK_GET || ltok == TOK_SET) ? Or maybe just (ltok != TOK_GET && ltok != TOK_SET) since we're basically handling things that are not get/set as accessor. That seems clearest to me. @@ +8956,5 @@ > return propName; > } > > + if (TokenKindIsPossibleIdentifierName(ltok) > + && (tt == TOK_COMMA || tt == TOK_RC || tt == TOK_ASSIGN)) && at end of previous line. ::: js/src/frontend/Parser.h @@ +877,5 @@ > > template <typename ParseHandler> > class Parser final : public ParserBase, private JS::AutoGCRooter > { > + Stray blank line. @@ +1472,5 @@ > +private: > + Parser<ParseHandler>* parser_; > + bool oldAwaitIsKeyword_; > + > +public: These access specifiers want to be indented two, and elsewhere in this patch. @@ +1481,5 @@ > + } > + > + ~AutoAwaitIsKeyword() { > + parser_->setAwaitIsKeyword(oldAwaitIsKeyword_); > + parser_ = nullptr; Doesn't seem necessary to null here, no? ::: js/src/frontend/TokenKind.h @@ +275,5 @@ > > +inline MOZ_MUST_USE bool > +TokenKindIsKeyword(TokenKind tt) > +{ > + return (tt >= TOK_KEYWORD_FIRST && tt <= TOK_KEYWORD_LAST) || I'd prefer BEGIN <= tt && tt <= END as general form, to read more like the specification of a range. ::: js/src/frontend/TokenStream.cpp @@ +182,5 @@ > bool > frontend::IsKeyword(JSLinearString* str) > { > + const ReservedWordInfo* rw = FindReservedWord(str); > + if (rw == nullptr) Could do if (const ReservedWordInfo* rw = FindReservedWord(str)) return TokenKindIsKeyword(rw->tokentype); return false; and for the others. @@ +228,5 @@ > + > +#define EMIT_RETURN(word, name, type) \ > + if (rw->tokentype == type) \ > + return js_##word##_str; > +FOR_EACH_JAVASCRIPT_RESERVED_WORD(EMIT_RETURN) Mm. I think I'd rather not rely on the compiler to figure out how to tableswitch this. Can we get some sort of switch for this, maybe, somehow? (And also in the next function.) @@ +1446,5 @@ > > + // Represent reserved words as reserved word tokens. > + if (!hadUnicodeEscape) { > + const ReservedWordInfo* rw = FindReservedWord(chars, length); > + if (rw) { if (const ...) { ::: js/src/jit-test/tests/modules/export-declaration.js @@ +434,3 @@ > assertThrowsInstanceOf(function() { > + parseAsModule("export { true }"); > +}, SyntaxError); It'd probably be good to separate out this fix, if at all possible, from the larger patch. ::: js/src/moz.build @@ +632,5 @@ > ] > > +GENERATED_FILES += ['jsautorw.h'] > +jsautorw = GENERATED_FILES['jsautorw.h'] > +jsautorw.script = 'jsautorw.py' I'd prefer ReservedWordsGenerated.{h,py} for naming -- we're trying to get rid of js* names, very slowly. Maybe in frontend/ if possible.
Attachment #8833732 - Flags: feedback?(jwalden+bmo) → feedback+
Oh, as a practical matter, conflict away with my modifier changes. Fixing the individual conflicts will be finicky but straightforward, I expect.
Thank you for your feedback :) (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4) > ::: js/src/frontend/Parser.cpp > @@ +4647,5 @@ > > > > if (tt == TOK_RC) > > break; > > > > + MUST_MATCH_TOKEN_COND(TokenKindIsPossibleIdentifierName(token), JSMSG_NO_IMPORT_NAME); > > I assume |token| doesn't actually compile here, right? And many other > places. I'm guessing this is just f? purposes, mostly? `token` is actually a part of the macro. > #define MUST_MATCH_TOKEN_COND_MOD(cond, modifier, errorNumber) \ > JS_BEGIN_MACRO \ > TokenKind token; \ > ... > if (!(cond)) { \ I guess it should be rewritten to be more readable, maybe passing function name instead of partial expression. > @@ +5018,5 @@ > > + // should contain IdentifierReference. > > + > > + for (ParseNode* next = kid->pn_head; next; next = next->pn_next) { > > + if (!next->isKind(PNK_EXPORT_SPEC)) > > + continue; > > I must be reading this wrong somehow, because it looks like the only > children appended to |kid| are *always* PNK_EXPORT_SPEC. So this loop never > does anything but iterate everything and continue on each node. Why isn't > this the case? But, I assume this loop fixes the additional test you added > to export-declaration.js, so I'm maybe confused here. > > Assuming this loop does actually do anything, I don't see why > identifier-checking can't be performed directly as the ExportClause is being > parsed. Disillusion me? yeah, the condition was unnecessary, and "continue" won't be reached. it checks if the ExportClause contains IdentifierReference instead of IdentifierName. looks like the spec I was referring was rewritten today :P https://github.com/tc39/ecma262/commit/9c0c0d2ffa39be4c0af01c8e9410a649440d636f I'll split that part into another patch for another bug, and also rewrite to follow the updated spec. > @@ +5121,3 @@ > > return null(); > > break; > > case TOK_CLASS: > > Apropos of nothing, we really need to split up Parser::exportDeclaration > into sub-functions to handle the various different possible syntaxes within > it. The shared three-ish line tail can't possibly justify it. will fix in another patch. also, looks like I forgot to handle `export async function(){}` :P > @@ +6846,5 @@ > > + // Avoid getting next token with None. > > + if (tt == TOK_AWAIT && pc->isAsync()) > > + return expressionStatement(yieldHandling); > > + > > + if (!TokenKindIsPossibleIdentifier(tt) && tt != TOK_LET) > > TokenKindIsPossibleIdentifier(TOK_LET) is true. So the second condition > there is, if evaluated, always true. I think the prior system here of > peeking, handling |let| if encountered and actually a declaration, then if > necessary falling through to the label/expression cases, was the proper one. > So some of this changing needs undoing. removed unnecessary "tt != TOK_LET" part. I added it just to explicitly exclude the cases that is handled below, but `!TokenKindIsPossibleIdentifier(tt)` already filtered that case out. in patched code, `TOK_AWAIT` and `!TokenKindIsPossibleIdentifier(tt)` cases handles unpatched-default clause, that just does `return expressionStatement(yieldHandling);`. also, remaining part handles unpatched-TOK_NAME case and some deferred error cases for reserved words. I don't see any difference. can you tell me more about the issue?
update from previous patch: * Renamed jsautokw.{py,h} => frontend/ReservedWordsGenerated.{py,h} * Added MUST_MATCH_TOKEN_INTERNAL that is used by the following macros: * MUST_MATCH_TOKEN * MUST_MATCH_TOKEN_MOD * MUST_MATCH_TOKEN_FUNC * MUST_MATCH_TOKEN_FUNC_MOD * Removed the fix for export, except removing wrong testcase that fails now
Assignee: nobody → arai.unmht
Attachment #8833732 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8835099 - Flags: review?(jwalden+bmo)
now Parser::checkStrictBinding is not necessary in most case, since bindingIdentifier checks the same thing before that point. but some cases ([A], [B], and [C] in the following) needs extra check. also extra warning should be shown, so changed labelOrIdentifierReference/bindingIdentifier to handle needStrictChecks and use strictModeErrorAt. here's reverse call tree for checkStrictBinding, and possible name value: checkStrictBinding notePositionalFormalParameter functionArguments <= bindingIdentifier(yieldHandling) noteDeclaredName functionStmt <= bindingIdentifier(yieldHandling) context->names().starDefaultStar checkDestructuringName <= expr->name() checkDestructuringObject <= destructuring object pattern item identifierReference(yieldHandling) [A] checkDestructuringArray <= destructuring array pattern item identifierReference(yieldHandling) [A] declarationName <= bindingIdentifier(yieldHandling) namedImportsOrNamespaceImport <= importedBinding() importedBinding() importDeclaration <= importedBinding() exportDeclaration <= context->names().starDefaultStar tryStatement <= bindingIdentifier(yieldHandling) classDefinition <= bindingIdentifier(yieldHandling) context->names().starDefaultStar comprehensionFor <= tokenStream.currentName() [B] functionFormalParametersAndBody <= fun->explicitName()->asPropertyName() [C] [A] need to check `arguments` and `eval` name. Moved the checking part of Parser::bindingIdentifier to Parser::checkBindingIdentifier, and added checkBindingIdentifier call before noteDeclaredName. [B] need to check if it's bindingIdentifier changed to bindingIdentifier(YieldIsKeyword), since the previous definition only allowed TOK_NAME. [C] some case needs bindingIdentifier check (see below) Replaced checkStrictBinding with checkBindingIdentifier call. possible source of fun->explicitName() JSFunction::atom_, not HAS_COMPILE_TIME_NAME, not HAS_GUESSED_ATOM initAtom XDRInterpretedFunction <= already checked createScriptForLazilyInterpretedFunction <= already checked NewFunctionWithProto <= *** at least this should be checked *** NewFunctionClone <= already checked getSelfHostedFunction <= already checked setAtom intrinsic_FinishBoundFunctionInit <= already checked intrinsic_SetCanonicalName <= never fail ParseFunction <= m.parser().bindingIdentifier(YieldIsName)
Attachment #8835102 - Flags: review?(jwalden+bmo)
Comment on attachment 8835099 [details] [diff] [review] Part 1: Rework on reserved word and remove TokenStream::KeywordIsName. Review of attachment 8835099 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/Parser.cpp @@ +66,5 @@ > JS_BEGIN_MACRO \ > TokenKind token; \ > if (!tokenStream.getToken(&token, modifier)) \ > return null(); \ > + if (!(cond)) { \ Instead of |cond|, make this take |func| and then do if (!(func)(token)) and at that point this macro can just be MUST_MATCH_TOKEN_FUNC and MUST_MATCH_TOKEN and MUST_MATCH_TOKEN_MOD can both invoke it. @@ +76,5 @@ > +#define MUST_MATCH_TOKEN(tt, errorNumber) \ > + MUST_MATCH_TOKEN_INTERNAL(token == tt, TokenStream::None, errorNumber) > + > +#define MUST_MATCH_TOKEN_MOD(tt, modifier, errorNumber) \ > + MUST_MATCH_TOKEN_INTERNAL(token == tt, modifier, errorNumber) The |func| passed for this macro and the one above can be [](TokenKind tok) { return tok == tt; } @@ +81,5 @@ > + > +#define MUST_MATCH_TOKEN_FUNC(func, errorNumber) \ > + MUST_MATCH_TOKEN_INTERNAL(func(token), TokenStream::None, errorNumber) > + > +#define MUST_MATCH_TOKEN_FUNC_MOD(func, modifier, errorNumber) \ If the parameter is |func|, these two macros can just go away entirely. @@ +792,5 @@ > +Parser<FullParseHandler>::setAwaitIsKeyword(bool isKeyword) > +{ > + awaitIsKeyword_ = isKeyword; > + Parser<SyntaxParseHandler>* parser = handler.syntaxParser; > + if (parser) if (Parser<>* parser = ...) @@ +951,5 @@ > + if (name == context->names().eval) > + return strictModeErrorAt(pos.begin, JSMSG_BAD_BINDING, "eval"); > + > + if (name == context->names().let) { > + error(JSMSG_RESERVED_ID, "let"); Don't these need to be |errorAt| to be consistent with the cases above? If for some bizarre reason they should be different, a comment is needed. ::: js/src/frontend/ReservedWordsGenerated.py @@ +1,1 @@ > # This Source Code Form is subject to the terms of the Mozilla Public Seems like GenerateReservedWords.py is a mildly better filename. ::: js/src/frontend/TokenStream.h @@ +344,5 @@ > JSVersion versionWithFlags() const { return options().version; } > > + private: > + PropertyName* > + reservedWordToPropertyName(TokenKind tt) const; This all fits on one line, right? @@ +351,2 @@ > PropertyName* currentName() const { > + if (!isCurrentTokenType(TOK_NAME)) { I'd prefer if (isCurrentTokenType(TOK_NAME)) return currentToken().name(); MOZ_ASSERT(TKIPIN(...)); return rWTPN(...); to have the common case read first. And in nextName() too.
Attachment #8835099 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8835102 [details] [diff] [review] Part 2: Remove Parser::checkStrictBinding. Review of attachment 8835102 [details] [diff] [review]: ----------------------------------------------------------------- Please test that the wrong testcase, removed in the previous patch, is readded here except with a check for a syntax error. I'm running a little short on time, but I'd rather stamp now than delay longer on this. We can do more in a followup to deal with the more involved comments here. ::: js/src/frontend/Parser.cpp @@ +8532,5 @@ > { > errorAt(offset, JSMSG_RESERVED_ID, "yield"); > return nullptr; > } > + if (pc->sc()->needStrictChecks()) { Hmm. I distinctly remember changing this code to check strictness, from the strict-checks behavior. (And adding a test that I can't find now.) I did so because the strict-checks behavior, in concert with werror, makes violations of this in lazily-parsed code into errors *at the time of full parsing*. That's no good, it's long after the actual parse, etc. For example, I'm fairly sure you'd break this: var yield = 42; function f() { yield = 5; } // parses options("strict"); options("werror"); f(); // delazifies |f|, tries to parse it, ERROR That said, it looks like this slight variant is *already* broken this way: function f() { var yield; } // parses options("strict"); options("werror"); f(); // delazifies |f|, tries to parse it, ERROR I guess you can go ahead with this, but we really need to follow up quickly and figure out *some* sort of way for this stuff to play well with lazy parsing. I'm not actually sure what that is -- maybe a way to tag the LazyScript to indicate that bad-name-warning already happened, and don't do it? Initially set to false, then set to true after the lazy-parse completes. Or something.
Attachment #8835102 - Flags: review?(jwalden+bmo) → review+
Thank you for reviewing :) (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10) > var yield = 42; > function f() { yield = 5; } // parses > options("strict"); > options("werror"); > f(); // delazifies |f|, tries to parse it, ERROR Is this possible scenario in web, or just an example that demonstrates more complicated issue? I don't think strictness can be changed at that point, without the testing function.
Pushed by arai_a@mac.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/476a938ee2ab Part 1: Rework on reserved word and remove TokenStream::KeywordIsName. r=jwalden https://hg.mozilla.org/integration/mozilla-inbound/rev/761227e357b9 Part 2: Remove Parser::checkStrictBinding. r=jwalden
lambda in MUST_MATCH_TOKEN_MOD hit internal compiler error on SM-tc(H): https://treeherder.mozilla.org/logviewer.html#?job_id=77609299&repo=mozilla-inbound&lineNumber=4995 changed to simpler one that just passes expression.
Attachment #8837642 - Flags: review?(jdemooij)
Comment on attachment 8837642 [details] [diff] [review] Part 3 - Use simpler macro to avoid internal compiler error on SM-tc(H). Review of attachment 8837642 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable and I'll stamp it to avoid a backout (hopefully), but maybe you can do some Try pushes to narrow it down more? I'm wondering whether parentheses around the lambda would help. ::: js/src/frontend/Parser.cpp @@ +79,5 @@ > #define MUST_MATCH_TOKEN(tt, errorNumber) \ > MUST_MATCH_TOKEN_MOD(tt, TokenStream::None, errorNumber) > > +#define MUST_MATCH_TOKEN_FUNC_MOD(func, modifier, errorNumber) \ > + MUST_MATCH_TOKEN_INTERNAL(func(token), modifier, errorNumber) Should we put parens around func here, like before?
Attachment #8837642 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9541461973eadf8ceab7422283211d643ecb24b5 Bug 1336783 - Part 3 - Use simpler macro to avoid internal compiler error on SM-tc(H). r=jandem CLOSED TREE
(In reply to Tooru Fujisawa [:arai] from comment #11) > (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10) > > var yield = 42; > > function f() { yield = 5; } // parses > > options("strict"); > > options("werror"); > > f(); // delazifies |f|, tries to parse it, ERROR > > Is this possible scenario in web, or just an example that demonstrates more > complicated issue? > I don't think strictness can be changed at that point, without the testing > function. It's not possible if we're looking solely at the web. However, as a matter of the APIs SpiderMonkey provides, it does exist as a concern. (Incidentally, I was thinking of the case of functions whose names are extra-warnings violations.) I think, if we're going to fix this concern, it probably has to be fixed system-wide: extraWarnings+werror needs to do all warning/erroring on the first parse, and never on delazifying parses. Will have to think about how to do this, but separate from this bug.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: