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 >
