2009/11/9 <[email protected]>: > Some detailed comments inline, but general comments below. I have not > finished going through ScopeAnalyzer.java yet. The broad questions are: > > 1. ScopeAnalyzer is parameterized by its scope implementation type, <S > extends AbstractScope>. Why should it constrain S to extend > AbstractScope given that it otherwise makes no demands on S? By firing > enterScope and exitScope on S, it requires the listener's type S to be > an AbstractScope, which the listener may not want.
I don't want the listener to have to typecast the scope objects it creates every time it handles an event. Is there another typesafe way to achieve that? > 2. That said, it's probably perfectly reasonable that most listeners > will construct *some* S that is an AbstractScope. So then there's the > opposite question. The ScopeAnalyzer is creating all this nifty richly > connected graph with the Symbol and ScopeTree objects. But, alack! -- > the listener is forbidden from seeing any of this. It gets created and > thrown away. Is there benefit to making this an output of the > ScopeAnalyzer? Not that I can see. > I guess it just seems to me that there are 2 easily understandable > choices -- complete separation or complete connection between Analyzer > and its listeners. The current code seems to be a bit of a mixture. Where is the connection? The analyzer merely routes scopes from the listener back to itself. It never invokes their methods. > http://codereview.appspot.com/148045/diff/1/11 > File src/com/google/caja/parser/js/scope/AbstractScope.java (right): > > http://codereview.appspot.com/148045/diff/1/11#newcode22 > src/com/google/caja/parser/js/scope/AbstractScope.java:22: public > interface AbstractScope { > If we *are* going to define such a beast, should it not also include a > slot for a pointer to the ParseTreeNode from which it is generated? It could. I think there is value in at least having a placeholder type for explanatory purposes, and we can get as rich or as simple as we like. > http://codereview.appspot.com/148045/diff/1/11#newcode23 > src/com/google/caja/parser/js/scope/AbstractScope.java:23: /** > Blank line before "/*" - all in this file. Done > http://codereview.appspot.com/148045/diff/1/6 > File src/com/google/caja/parser/js/scope/ScopeAnalyzer.java (right): > > http://codereview.appspot.com/148045/diff/1/6#newcode93 > src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:93: protected > abstract boolean fnCtorsDeclareInBody(); > Should this simply be a boolean parameter to the ScopeAnalyzer ctor? It could be a parameter or it could be an overridable method. > http://codereview.appspot.com/148045/diff/1/6#newcode98 > src/com/google/caja/parser/js/scope/ScopeAnalyzer.java:98: protected > abstract boolean fnCtorsDeclareInContaining(); > Same. > > http://codereview.appspot.com/148045/diff/1/7 > File src/com/google/caja/parser/js/scope/ScopeListener.java (right): > > http://codereview.appspot.com/148045/diff/1/7#newcode20 > src/com/google/caja/parser/js/scope/ScopeListener.java:20: public > interface ScopeListener<S extends AbstractScope> { > This interface reads a little confusingly to me. Is type S the type of > scopes being constructed by the listener? If not, then it must be the > scopes constructed by the scope analyzer, right? If so, then the type of > *these* scopes is fixed and it need not be a type parameter. The scope analyzer does not construct AbstractScopes. > To clear this up and separate concerns more crisply, I think you could > get away with not introducing a Scope in the signature of ScopeListener > at all if your callbacks refer, not to a Scope, but to the ParseTreeNode > corresponding to the Scope. > This change would not cause more objects to be created or more code to > be run. The objects of type S being given to this listener are *still* > being created by the scope analyzer during its tree walk, and the > listener is *still* creating its own custom scopes if it wants to. And now a whole bunch of listeners have to keep an identity hash map of ParseTreeNodes to scopes. But that would not work, because ParseTreeNodes might appear multiple times in the same AST. > http://codereview.appspot.com/148045/diff/1/7#newcode29 > src/com/google/caja/parser/js/scope/ScopeListener.java:29: void > declaration(AncestorChain<Identifier> id, S scope); > For example: > > declaration(..., ParseTreeNode node) > > where the node would be (say) a FunctionConstructor, a CatchStmt, an > UncajoledModule, .... In other words, some node that *introduces* a new > scope, but not the scope itself (building that is the job of the > listener, if it wants to). > > http://codereview.appspot.com/148045/diff/1/9 > File src/com/google/caja/parser/js/scope/ScopeType.java (right): > > http://codereview.appspot.com/148045/diff/1/9#newcode34 > src/com/google/caja/parser/js/scope/ScopeType.java:34: /** > More spaces before "/*" > > http://codereview.appspot.com/148045 >
