Summary: | [ES6] Implement tagged templates | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, darin, fpizlo, ggaren, gyuyoung.kim, oliver, ossy, rniwa | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 142691, 144330, 145013, 145019, 145248 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Yusuke Suzuki
2015-03-28 02:32:54 PDT
Created attachment 252767 [details]
Patch
Comment on attachment 252767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252767&action=review r-, but literally just for the tweaks mentioned above. > Source/JavaScriptCore/parser/Parser.h:781 > + template <class TreeBuilder> NEVER_INLINE typename TreeBuilder::TemplateLiteral parseTemplateLiteral(TreeBuilder&, bool shouldBuildRawStrings); Can you replace this bool with a clearly named enum? In general we've shied away from opaque bool parameters as you can't necessarily tells what's going on -- especially in the earlier call 'parseTemplateLiteral(context, false) Overall it looks good though. > Source/JavaScriptCore/parser/SyntaxChecker.h:85 > + TemplateStringResult, TemplateStringListResult, TemplateExpressionListResult, TemplateExpr, TaggedTemplateExpr i'd split the line at this point Created attachment 253114 [details]
Patch
Comment on attachment 252767 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252767&action=review Thank you for your review :D >> Source/JavaScriptCore/parser/Parser.h:781 >> + template <class TreeBuilder> NEVER_INLINE typename TreeBuilder::TemplateLiteral parseTemplateLiteral(TreeBuilder&, bool shouldBuildRawStrings); > > Can you replace this bool with a clearly named enum? In general we've shied away from opaque bool parameters as you can't necessarily tells what's going on -- especially in the earlier call 'parseTemplateLiteral(context, false) > > Overall it looks good though. Make sense! OK, I've introduced enum class RawStringsBuildMode { BuildRawStrings, DontBuildRawStrings }. >> Source/JavaScriptCore/parser/SyntaxChecker.h:85 >> + TemplateStringResult, TemplateStringListResult, TemplateExpressionListResult, TemplateExpr, TaggedTemplateExpr > > i'd split the line at this point Splitted into TemplateStringResult, TemplateStringListResult, TemplateExpressionListResult, TemplateExpr, TaggedTemplateExpr Comment on attachment 253114 [details] Patch Clearing flags on attachment: 253114 Committed r184337: <http://trac.webkit.org/changeset/184337> All reviewed patches have been landed. Closing bug. (In reply to comment #5) > Comment on attachment 253114 [details] > Patch > > Clearing flags on attachment: 253114 > > Committed r184337: <http://trac.webkit.org/changeset/184337> It caused zillion failures on the debug bots. Don't you watch the bots after landing patches? You should. (In reply to comment #7) > (In reply to comment #5) > > Comment on attachment 253114 [details] > > Patch > > > > Clearing flags on attachment: 253114 > > > > Committed r184337: <http://trac.webkit.org/changeset/184337> > > It caused zillion failures on the debug bots. Don't > you watch the bots after landing patches? You should. Ah, currently investigating...... Sorry for troubling you. debug builds become green. Now investigating EFL builld failures in ARM builds. https://build.webkit.org/builders/EFL%20Linux%20ARMv7%20Thumb2%20Release?numbuilds=25 Opened for EFL ARM build failures. https://bugs.webkit.org/show_bug.cgi?id=145019 Green comes back. |