Addressed in CL 116114 since this was committed.
http://codereview.appspot.com/110096/diff/4003/4006
File src/com/google/caja/lexer/Keyword.java (right):
http://codereview.appspot.com/110096/diff/4003/4006#newcode118
Line 118: public static boolean isKeyword(String identifier) {
On 2009/09/15 18:31:58, DavidSarah wrote:
"isKeyword(String name)"
The argument need not be an identifier (and strictly speaking,
keywords are not
identifiers).
Done.
http://codereview.appspot.com/110096/diff/4003/4009
File src/com/google/caja/parser/quasiliteral/AlphaRenaming.java (right):
http://codereview.appspot.com/110096/diff/4003/4009#newcode90
Line 90: * the set of free variables of any given expression.
On 2009/09/15 18:31:58, DavidSarah wrote:
"set of free variable names"
Changed to set of free identifiers since it should apply equally well to
const decls.
http://codereview.appspot.com/110096/diff/4003/4009#newcode103
Line 103: * rename arbitrary javascript parse trees, but there is an
added wrinkle that
On 2009/09/15 18:31:58, DavidSarah wrote:
"JavaScript"
Done.
http://codereview.appspot.com/110096/diff/4003/4009#newcode146
Line 146: // Check that the rewriter was correct by creating a scope
that declares
On 2009/09/15 18:31:58, DavidSarah wrote:
"Perform a sanity check on the rewriter output", not "check that the
rewriter
was correct".
Quite right. Done
http://codereview.appspot.com/110096/diff/4003/4009#newcode172
Line 172: Set<String> freeVars = Sets.newLinkedHashSet();
On 2009/09/15 18:31:58, DavidSarah wrote:
freeVarNames
Done but with freeIdents since equally applicable to const uses.
http://codereview.appspot.com/110096/diff/4003/4009#newcode178
Line 178: for (String freeVar : freeVars) {
On 2009/09/15 18:31:58, DavidSarah wrote:
freeVarName : freeVarNames
Done.
http://codereview.appspot.com/110096/diff/4003/4008
File src/com/google/caja/parser/quasiliteral/AlphaRenamingRewriter.java
(right):
http://codereview.appspot.com/110096/diff/4003/4008#newcode142
Line 142: // masks the function name, then don't clobber it.
On 2009/09/15 18:31:58, DavidSarah wrote:
Unclear which cases this applies to. Maybe an example would help.
Done.
http://codereview.appspot.com/110096/diff/4003/4007
File src/com/google/caja/parser/quasiliteral/NameContext.java (right):
http://codereview.appspot.com/110096/diff/4003/4007#newcode90
Line 90: public VarInfo<NAME, BINDING> declare(NAME origName,
FilePosition declSite)
On 2009/09/15 18:31:58, DavidSarah wrote:
'declare' means introduce a new declaration (as opposed to renaming an
existing
declaration), right? I think the methods of this class need more
documentation.
Yes. Commented.
http://codereview.appspot.com/110096/diff/4003/4012
File src/com/google/caja/util/SafeIdentifierMaker.java (right):
http://codereview.appspot.com/110096/diff/4003/4012#newcode64
Line 64: public boolean isSafeIdentifier(String ident) {
On 2009/09/15 18:31:58, DavidSarah wrote:
Is this duplicating checks elsewhere in the cajoler?
There are checks that identifiers are valid.
But this is necessary because otherwise, AlphaRenaming might take in
code that passes those checks and produce code that fails them.
http://codereview.appspot.com/110096/diff/4003/4012#newcode86
Line 86: assert j < e;
On 2009/09/15 18:31:58, DavidSarah wrote:
Why not j <= e?
Yes. If we wanted to produce the valid but silly identifier series:
a aa aaa aaaa ...
then charRanges would be called charRanges('a', 'a').
Changed.
http://codereview.appspot.com/110096
