I'm taking a look at this now. It would be helpful if you could update your playpen and re-run the review tool (so it checks in an up-to-date changeset). You don't have to send the mail, just let me know when you've updated the change set and I will re-fetch it.
On 2011-03-03, at 10:46, Donald Anderson wrote: > (Re-review with fixes for Andre's test cases): > > Change dda-20110301-0dx by [email protected] on 2011-03-01 14:23:55 EST > in /Users/dda/laszlo/src/svn/openlaszlo/trunk-c/WEB-INF/lps/schema > for http://svn.openlaszlo.org/openlaszlo/trunk/WEB-INF/lps/schema > > Summary: Eliminate the need for with(this) from most typical cases > > New Features: > > Bugs Fixed: LPP-8751 Eliminate the use of `with(this)` > > Technical Reviewer: ptw (pending) > QA Reviewer: henry (pending) > Doc Reviewer: (pending) > > Documentation: > > Release Notes: > > Overview: > with(this) can be eliminated from a function only when either of these > conditions occur: > - we have an complete knowledge of the class heirarchy. > - there are no free references in the function that cannot be > accounted for as globals. > and when this condition does not occur: > - another 'with(varname)' appears in the function. > We address the first condition via changes to the js2doc as it > generates the lfc schema, the tag compiler as it generates extra > 'reference' JS classes that contain attributes and methods with > empty implementations, and the script compiler, as it uses the > reference classes to create a (usually) complete picture of the class > heirarchy for most cases. The second condition is addressed via > a prescan of the AST for globals and class names. As for the third > condition, there are ways of mitigating it, but we don't bother for now > on the assumption that a function coded containing 'with(varname)' is not > a function that is coded for speed. > > There may be some future work that may require a complete set of > reference classes, or method args, etc.. These are mentioned in > comments below as 'FUTURE WORK'. For most of these, preliminary > explorations were done, and abandoned for expediency. > > Details: > [Four files changed since 3/1 review sent: > CommonGenerator.java, JavascriptGenerator.java, > WithThisAnalyzer.java, lztest-class-impl.lzx] > > test/lztest/lztest-class-impl.lzx: > Added some simple tests that illustrate how with(this) is inserted > in certain cases (before this change). By inspecting JS, used to > verify that 'this.' insertion and 'with(this)' removal is working. > > WEB-INF/lps/schema/lfc-undeclared.lzx: > Added attribute: isinited, marked as private in doc, which is used > by old components, and cannot be marked as public. > Added method: setAttribute, which actually lives in LzEventable, and is > used quite often. We don't current emit reference classes for > any classes that are not associated with a tag (it was too hard > to get a non-tag class into the tag compiler - FUTURE WORK). > > WEB-INF/lps/lfc/core/LzNode.lzs: > Added devnote about isinited. > > WEB-INF/lps/lfc/compiler/Class.lzs: > Minor error message improvement that I found helpful once. (optional) > > WEB-INF/lps/server/src/org/openlaszlo/js2doc/SchemaBuilder.java: > - jsname (the javascript name) for a class as well as jsextends > (the javascript class it extends) are put into the schema, though > not currently used - FUTURE WORK). > - omit event names not already defined as attributes > - method parameters are now collected and inserted into the schema, > but no typing is currently preserved (FUTURE WORK). > > WEB-INF/lps/server/src/org/openlaszlo/compiler/ClassCompiler.java: > Ignore classes in the schema that has no associated tag name (FUTURE > WORK). > Small change to the protocol of compiling a ClassModel. > > WEB-INF/lps/server/src/org/openlaszlo/compiler/NodeModel.java: > Ignore the jsname, jsextends parts of the schema (FUTURE WORK). > Small change to the protocol of compiling a ClassModel. > > WEB-INF/lps/server/src/org/openlaszlo/compiler/ClassModel.java: > - Changed isCompiled() to be true to its name. > - reworked compile() to handle emiting reference classes as needed. > - reworked emitClassDeclaration() to work with reference classes. > > WEB-INF/lps/server/src/org/openlaszlo/sc/ScriptClass.java: > Changed generation of a class so it won't create attributes named 'with' > classes named 'class', etc. Needed when generating all classes for > the full schema (possibly optional now, but needed for FUTURE WORK). > > WEB-INF/lps/server/src/org/openlaszlo/sc/CommonGenerator.java: > - rearrange globals and debugGlobals into separate sets, enumerate > the members of JS 'Object' class. > - ClassDescriptor.getInstanceProperties() modified to return an > 'incomplete' set, > rather than null. ClassDescriptor.complete is set and needs to be > checked > by the caller. > - ClassDescriptor.incompleteSet() added for informational messages. > - ClassDescriptor.toString() for development/debugging. > - process reference classes minimally when needed at all. > > WEB-INF/lps/server/src/org/openlaszlo/sc/JavascriptGenerator.java: > - create set of forwardGlobals by doing a shallow walk of the AST. > - manage the 'possibleInstance' set to try to reduce it and move > as many items as appropriate/possible to the 'bindthis' set, > which is the set of variables we will actively bind to 'this'. > - issue warning if 'warnWithThis' is on. > > WEB-INF/lps/server/src/org/openlaszlo/sc/SWF9Generator.java: > Make sure reference classes are ignored in this generator. > > WEB-INF/lps/server/src/org/openlaszlo/sc/DHTMLCompiler.java: > set option so that reference classes are accepted. > > WEB-INF/lps/server/src/org/openlaszlo/sc/Compiler.java: > define 'referenceClass' pragma issued by tag compiler. > define 'registerReferenceClasses', an option only set by the > DHTMLCompiler. > define 'warnWithThis', a new debugging option that will warn > when with(this) is used, and give the reason behind it, e.g.: > 'with(this) added in foobar: unknown parts of class hierarchy: > [LzTestManagerClass->LzEventable] and unaccounted refs: [LzHTTPLoader, > escape]' > > WEB-INF/lps/server/src/org/openlaszlo/sc/CodeGenerator.java: > Abide by new ClassDescriptor protocol > > WEB-INF/lps/server/src/org/openlaszlo/sc/GenericVariableAnalyzer.java: > WEB-INF/lps/server/src/org/openlaszlo/sc/VariableAnalyzer.java: > WEB-INF/lps/server/src/org/openlaszlo/sc/WithThisAnalyzer.java: > Refactored VariableAnalyzer so essential parts could be shared > with a new 'WithThisAnalyzer'. The new analyzer walks the tree > and applies identifier --> this.identifier transformation for > any free variables found in a set. It also tracks if any > 'with' constructs are found, which is a spoiler for with(this) removal. > > WEB-INF/lps/server/sc/src/org/openlaszlo/sc/parser/SimpleNode.java: > Added indexOf() - a convenience function, helpful in the 'this binding'. > > Tests: > - Ran smoketest (DHTML,SWF10) lztest, runlzunit > - after lztest, examined *.js files left behind to determine that > 79% (286 of 362) of occurrences of with(this) had been removed compared > with a run without these changes. Spot checked to ensure that 'this.' > was inserted, specifically in the 'WithThis' tests > (in test/lztest/lztest-class-impl.lzx) > - Of the occurrences left, we know why they occur: 1) LzTestManagerClass > inherits > from LzEventable, not a class associated with a tag. This will not happen > for > pure lzx programs. 2) they often refer to use class names in the LFC: > LzHTTPLoader, LzTimeKernel. 3) they refer to identifiers 'canvas', > 'escape'. > > > > Files: > M test/lztest/lztest-class-impl.lzx > M WEB-INF/lps/schema/lfc-undeclared.lzx > M WEB-INF/lps/lfc/core/LzNode.lzs > M WEB-INF/lps/lfc/compiler/Class.lzs > M WEB-INF/lps/server/src/org/openlaszlo/js2doc/SchemaBuilder.java > A WEB-INF/lps/server/src/org/openlaszlo/sc/WithThisAnalyzer.java > M WEB-INF/lps/server/src/org/openlaszlo/sc/JavascriptGenerator.java > A WEB-INF/lps/server/src/org/openlaszlo/sc/GenericVariableAnalyzer.java > M WEB-INF/lps/server/src/org/openlaszlo/sc/VariableAnalyzer.java > M WEB-INF/lps/server/src/org/openlaszlo/sc/SWF9Generator.java > M WEB-INF/lps/server/src/org/openlaszlo/sc/CommonGenerator.java > M WEB-INF/lps/server/src/org/openlaszlo/sc/DHTMLCompiler.java > M WEB-INF/lps/server/src/org/openlaszlo/sc/Compiler.java > M WEB-INF/lps/server/src/org/openlaszlo/sc/CodeGenerator.java > M WEB-INF/lps/server/src/org/openlaszlo/sc/ScriptClass.java > M WEB-INF/lps/server/src/org/openlaszlo/compiler/ClassCompiler.java > M WEB-INF/lps/server/src/org/openlaszlo/compiler/NodeModel.java > M WEB-INF/lps/server/src/org/openlaszlo/compiler/ClassModel.java > M WEB-INF/lps/server/sc/src/org/openlaszlo/sc/parser/SimpleNode.java > > Changeset: http://svn.openlaszlo.org/openlaszlo/patches/dda-20110301-0dx.tar > > > > -- > > Don Anderson > Java/C/C++, Berkeley DB, systems consultant > > voice: 617-306-2057 > email: [email protected] > www: http://www.ddanderson.com > blog: http://libdb.wordpress.com > > > > >
