Bug 143183

Summary: [ES6] Implement tagged templates
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: 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 Flags
Patch
none
Patch none

Description Yusuke Suzuki 2015-03-28 02:32:54 PDT
Implement tagged templates, like

String.raw`Hello ${world}`
Comment 1 Yusuke Suzuki 2015-05-08 21:22:07 PDT
Created attachment 252767 [details]
Patch
Comment 2 Oliver Hunt 2015-05-13 15:27:15 PDT
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
Comment 3 Yusuke Suzuki 2015-05-14 01:48:10 PDT
Created attachment 253114 [details]
Patch
Comment 4 Yusuke Suzuki 2015-05-14 01:53:23 PDT
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 5 WebKit Commit Bot 2015-05-14 09:09:23 PDT
Comment on attachment 253114 [details]
Patch

Clearing flags on attachment: 253114

Committed r184337: <http://trac.webkit.org/changeset/184337>
Comment 6 WebKit Commit Bot 2015-05-14 09:09:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Csaba Osztrogonác 2015-05-14 11:45:54 PDT
(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.
Comment 8 Yusuke Suzuki 2015-05-14 11:46:47 PDT
(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.
Comment 9 Yusuke Suzuki 2015-05-14 12:01:08 PDT
Opened https://bugs.webkit.org/show_bug.cgi?id=145013.
Comment 10 Yusuke Suzuki 2015-05-14 13:44:45 PDT
debug builds become green.
Now investigating EFL builld failures in ARM builds.
https://build.webkit.org/builders/EFL%20Linux%20ARMv7%20Thumb2%20Release?numbuilds=25
Comment 11 Yusuke Suzuki 2015-05-14 14:03:30 PDT
Opened for EFL ARM build failures. https://bugs.webkit.org/show_bug.cgi?id=145019
Comment 12 Yusuke Suzuki 2015-05-14 16:53:42 PDT
Green comes back.