snapshotted
http://codereview.appspot.com/110096/diff/3023/3028
File src/com/google/caja/parser/quasiliteral/AlphaRenaming.java (right):
http://codereview.appspot.com/110096/diff/3023/3028#newcode48
Line 48: * that has no masking identifiers, and that only has free
variables within an
On 2009/09/10 04:11:19, ihab.awad wrote:
What is a "masking identifier"?
An identifier I' masks another identifier I if I' has the same name as
I, and I' is defined in a scope that is wholly contained by the scope in
which I is defined.
So the inner x masks the outer x.
var x = 4;
return (function (x) {
return x;
})(3);
After alpha renaming, this might become
var a = 4;
return (function (b) {
return b;
})(3);
http://codereview.appspot.com/110096/diff/3023/3028#newcode65
Line 65: * non-synthetic identifiers is trivial.
On 2009/09/10 04:11:19, ihab.awad wrote:
You seem to be describing some process but I don't get the flow.
Perhaps a small
worked example would help? Also see my questions below about the API
-- perhaps
an example here would clarify?
See the unittests for examples, and examples at
http://en.wikipedia.org/wiki/AlphaRenaming
http://codereview.appspot.com/110096/diff/3023/3028#newcode75
Line 75: * used to generate new names for local variables.
On 2009/09/10 04:11:19, ihab.awad wrote:
Is this description correct? If so, I don't understand it.
It is correct but unclear. I rewrote it.
http://codereview.appspot.com/110096/diff/3023/3028#newcode81
Line 81: Expression e, NameContext<String, ?> ctxt, Set<String>
freeSynthetics,
On 2009/09/10 04:11:19, ihab.awad wrote:
First of all, I would have expected "ctxt" and "freeSynthetics" to be
named,
respectively, something like "renamableEntryPoints" and
"nonRenamableEntryPoints" -- since that seems to be the distinction of
interest
Those names sound fine, except that they're identifiers, not points.
for the purposes of this class. If that would not be a useful
nomenclature, then
I misunderstand the use case; perhaps that should be clarified in the
class or
method docs.
That said, and more broadly, I don't understand the need for the
"ctxt"
argument. Perhaps other code (yet to be submitted for review)
clarifies this,
I hope my rewrite of @param ctxt clarifies that.
but it would be nice if this class could explain itself all by its
lonesome. As
I see it, we have some chunk of code like the following, where the
stuff in
double brackets is subject to renaming:
var a = 1, b;
b = [[ (function(theArg) { return theArg + 1; })(a); ]]
where I expect "a" and "b" are not renameable, since they are provided
by the
stuff in which the [[ ... ]] expression is embedded, but "theArg" can
be renamed
to something.
So it's similar to that but a little more complex. I added an example
at the top of the file to clarify.
If so, then all we need for the API of this class here is just
"freeSynthetics"
(aka "nonRenamableEntryPoints"), right? Or not? If not, why?
No, because ctxt provides a mapping of old names to new, not just a set
of allowed names.
And finally, if the variables in the embedding ("a" and "b" in my
example) are
themselves being alpha renamed, then why does this class not operate
on
something bigger than just the teeny little Expression? Why not save
all the
alpha renaming until you have a whole chunk (like, a Block or a
Program or what
not) and run the renaming on that? This would presumably ensure that
everything
got renamed in one go and all you'd need would be "freeSynthetics"
(aka
"nonRenamableEntryPoints").
Because when generating a program by building structure around user
supplied code, you have to be able to declare identifiers in the
structure code and have some guarantees that the user-defined snippets
of code can't muck with those identifiers you declare. If you generate
all the code and then do the renaming, it's simply too late.
http://codereview.appspot.com/110096/diff/3023/3028#newcode138
Line 138: if (!outers.contains(name) && isOuter(name, s)) {
On 2009/09/10 04:11:19, ihab.awad wrote:
Don't need "!outers.contains(name)" since "outers" is a Set.
Done.
http://codereview.appspot.com/110096/diff/3023/3028#newcode164
Line 164: final class AlphaRenamingRewriter extends Rewriter {
On 2009/09/10 04:11:19, ihab.awad wrote:
It would be nice if this were in a separate file....
Done.
http://codereview.appspot.com/110096/diff/3023/3027
File src/com/google/caja/parser/quasiliteral/NameContext.java (right):
http://codereview.appspot.com/110096/diff/3023/3027#newcode32
Line 32: * scoped, so we alpha-rename all variables to prevent
collisions.
On 2009/09/10 04:11:19, ihab.awad wrote:
"... we alpha-rename all variables ..." seems overly general; do you
mean only
the variables introduced by the IHTML?
Nope. We rename them all.
http://codereview.appspot.com/110096/diff/3023/3027#newcode34
Line 34: * @param <NAME> a type that can work as a hashtable key
On 2009/09/10 04:11:19, ihab.awad wrote:
Also describe <BINDING> param.
Done.
http://codereview.appspot.com/110096/diff/3023/3027#newcode82
Line 82: public NameContext<NAME, BINDING> subScope() {
On 2009/09/10 04:11:19, ihab.awad wrote:
Rename this method to "newChildContext" or "makeChildContext"?
Especially to
make it complementary to "getParentContext".
Done.
http://codereview.appspot.com/110096/diff/3023/3031
File src/com/google/caja/util/SafeIdentifierMaker.java (right):
http://codereview.appspot.com/110096/diff/3023/3031#newcode38
Line 38: this(charRanges('a', 'z', 'A', 'Z'));
On 2009/09/10 04:11:19, ihab.awad wrote:
Given that a larger alphabet means "denser" and shorter identifers, it
behooves
one to choose the largest alphabet possible. If you make
"isSafeIdentifier"
reject ones starting with a digit, you can use '0'..'9' as well,
boosting your
|alphabet| from 54 to 64. Perhaps a few other ASCII characters could
be
similarly pressed into service?
Sure. I want to make sure we don't output any identifiers that end with
"__", so maybe we should keep '_' out of the list.
The problem with skipping 0-9 for first character positions and not
subsequent is that we generate a string and reverse it so that we get a
nice lexicographic progression. That means that we don't know which
letter is the last until we generate it. There's probably an algorithm
that does that but it's not a simple variation on this one.
http://codereview.appspot.com/110096/diff/3023/3031#newcode70
Line 70: private static char[] charRanges(char... startsAndEnds) {
On 2009/09/10 04:11:19, ihab.awad wrote:
"assert((startsAndEnds %2) == 0)"? For documentation more than
anything else....
Done and documented the method.
http://codereview.appspot.com/110096/diff/3023/3024
File tests/com/google/caja/parser/quasiliteral/AlphaRenamingTest.java
(right):
http://codereview.appspot.com/110096/diff/3023/3024#newcode303
Line 303: private void assertRenamed(String golden, String input,
String... globals)
On 2009/09/10 04:11:19, ihab.awad wrote:
Based on the rest of our tests, I'm used to the input coming first,
then the
expected output. It was hard to read the tests until I reoriented
myself.
The signature for assertEquals is
void assertEquals(Object expected, Object actual)
and anybody reversing the arguments is wrong.
See
http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertEquals(java.lang.Object,%20java.lang.Object)
http://codereview.appspot.com/110096
