Ok ... still LGTM :) On Tue, Sep 15, 2009 at 6:27 AM, Mike Samuel <[email protected]> wrote: > 2009/9/14 <[email protected]>: >> LGTM >> >> >> http://codereview.appspot.com/110096/diff/2030/3041 >> File src/com/google/caja/parser/quasiliteral/AlphaRenaming.java (right): >> >> http://codereview.appspot.com/110096/diff/2030/3041#newcode62 >> Line 62: * not to conflict with generated code names. >> Perhaps explain why this class takes an Expression and not something >> else? Can just cut & paste from your review comments to me? ;) >> >> http://codereview.appspot.com/110096/diff/2030/3041#newcode132 >> Line 132: Expression e, NameContext<String, ?> ctxt, Set<String> >> freeSynthetics, >> So you don't think it's a good idea to rename "ctxt" and >> "freeSynthetics" (whatever we end up calling them)? > > I do think it's a good idea. I just didn't want to rename them until > we'd come to consensus on naming. > >> http://codereview.appspot.com/110096/diff/2030/3040 >> File src/com/google/caja/parser/quasiliteral/AlphaRenamingRewriter.java >> (right): >> >> http://codereview.appspot.com/110096/diff/2030/3040#newcode60 >> Line 60: matches="/* Reference */ @r", >> /* Expression */ ?? >> >> http://codereview.appspot.com/110096/diff/2030/3036 >> File tests/com/google/caja/parser/quasiliteral/AlphaRenamingTest.java >> (right): >> >> http://codereview.appspot.com/110096/diff/2030/3036#newcode63 >> Line 63: public final void testPropertNames() throws Exception { >> testPropert*y*Names >> >> http://codereview.appspot.com/110096/diff/2030/3036#newcode104 >> Line 104: ); >> Move closing paren 1 line up? >> >> http://codereview.appspot.com/110096/diff/2030/3036#newcode113 >> Line 113: + " var c = b;" >> Fwiw, the reason for this rewriting -- the addition of the function name >> as a "var" -- was not clear to me from reading the code. I suspect it >> has to do with trying to regularize the underlying semantics of function >> name scoping in JS ... but how precisely? By capturing the reference to >> the enclosing function even if that symbol is assigned to later on in >> the containing scope? >> >> http://codereview.appspot.com/110096/diff/2030/3036#newcode178 >> Line 178: + " function c() {" >> What is the rationale for not coming up with *yet* another variable >> here, so that it would be -- >> >> (function () { >> function b() { >> var c = b; >> function d() { >> var e = d; >> return e, a; >> } >> } >> return b; >> }) >> >> ? >> >> http://codereview.appspot.com/110096/diff/2030/3036#newcode233 >> Line 233: + " function c(e) { var d = c; return e * e; }" >> We should have a TODO for optimizing out the function name assignment >> "var" if that name is unused. It would be too messy to do now but maybe, >> in a future society, we would have a scope representation that would >> allow that to be done simply. >> >> http://codereview.appspot.com/110096 >> >
-- Ihab A.B. Awad, Palo Alto, CA
