Hi all, excellent - I'll sponsor the change.
Best, Michael > Am 23.10.2015 um 11:28 schrieb Attila Szegedi <attila.szeg...@oracle.com>: > > +1 from me as well. Another apology from me for having to wait even more for > the second review. > > Very nice work. I’m sometimes in two minds about RuntimeNode (especially the > fact that it currently can’t receive primitive parameters), but I have to > admit that in this particular case it made the integration of the feature > into the runtime fairly easy; it was really the parser parts that were tricky. > > Attila. > >> On Sep 24, 2015, at 4:44 PM, Hannes Wallnoefer >> <hannes.wallnoe...@oracle.com> wrote: >> >> Hi Andreas, >> >> Thanks for the contribution, and sorry for the long time it took to get back >> to you. >> >> I like the way you implemented this feature, the code looks very good. >> Comments inlined below. >> >> Am 2015-09-03 um 18:49 schrieb Andreas Woess: >>> Template literals are always scanned as a whole, quote-to-quote (as with >>> EditStringLexer). This turned out to be a problem with embedded functions >>> in expressions and lazy/optimistic compilation on: Parser#skipFunctionBody >>> would skip over the body and restart lexing at the RBRACE, forgetting about >>> the embedding string context. I've worked around this in skipFunctionBody >>> by skipping over to the RBRACE directly if it is already in the token >>> stream (which it is because we've already scanned to the end quote). >> >> It took me some time to figure this out. Maybe some more explanatory >> comments would be good. Does this also apply in other cases? >> >>> >>> Outstanding correctness issues not dealt with: >>> * 12.2.9.3 GetTemplateObject stipulates that the returned template object >>> be cached and unique. I don't know why you'd want the spec to demand >>> caching rather than allow it (functionally it does not make a difference, >>> but you could observe it not being cached in a test). >> >> Actually object identity can be observed by the tag function, and that was >> the reason for the committee to specify this. They chose caching of objects >> for performance reasons. The discussion can be found here: >> >> https://github.com/rwaldron/tc39-notes/blob/master/es6/2014-11/nov-18.md#48-template-literal-call-site-object-caching >> >> Even though it's a minor issue we should follow the spec here. >> >>> * 12.2.9.5 Evaluation: string concatenation using + is slightly off-spec. >>> There are two way to solve this: (a) wrap the expressions in a ToString >>> UnaryExpression (or RuntimeCall) or (b) generate a call to concat with >>> interleaved string and expression arguments. >>> >> >> The difference is between ToPrimitive being called with Number hint vs. >> String hint. This should not make a difference for the vast share of objects >> (all except those having a custom valueOf function I think), but it's >> something we should also get right. I've started experimenting with the >> solutions you suggested, I think adding a ToString conversion wrapper of >> some kind would be best. >> >> All in all I think your patch looks good. We can probably file the issues as >> separate bugs and fix them later. One thing I want to do is add some more >> tests. Although your test script covers a lot of stuff (for its size) I >> would like to add a bit to it. >> >> Thanks, >> Hannes -- <http://www.oracle.com/> Dr. Michael Haupt | Principal Member of Technical Staff Phone: +49 331 200 7277 | Fax: +49 331 200 7561 Oracle Java Platform Group | LangTools Team | Nashorn Oracle Deutschland B.V. & Co. KG, Schiffbauergasse 14 | 14467 Potsdam, Germany <http://www.oracle.com/commitment> Oracle is committed to developing practices and products that help protect the environment
