Comments:
1. ClassModel.java: Style nit:
if (! modelOnly || isbuiltin) {
I like to fully parenthesize expressions like this, to a. call attention to the
nearly invisible `not` operator, and b. make it so the human reading the code
does not have to remember the associativity rules for the language to read the
code.
2. SchemaBuilder#757: We have a /de facto/ situation where attribute types are
XML (or LZX) types, for the most part, with a magic mapping from XML type to
Javascript type, but method argument types are always just Javascript types. I
don't have any good proposals on how to fix that, but maybe what is really
important here is that if the LFC is advertising a public attribute with a type
that cannot be represented in the schema, perhaps it should not be a public
attribute after all? Maybe we should be issuing a warning for these attributes
(and clean them up).
3. CommonGenerator: "predefinedGlobals" are actually properties of the
ECMAScript global object, so perhaps should have a name that conveys that.
4. CommonGenerator: I wonder if it will be important to know the types of
built-in properties eventually?
5. JavascriptGenerator#302: I wonder if it would be better to warn (or err)
when Debug is referenced in non-debug code? That would seem to go along with
the goal of improved performance. In fact, rather than warnWithThis, I think I
would rather have the default to be if the compiler cannot resolve a reference
that it issue a warning (or error) and we provide a pragma to declare a
reference as being global (if it is an instance reference, we can silence the
warning with an explicit `this.`). We can't just do this unilaterally, and it
probably should not go in this change, but could you file an improvement to
make this happen? It will mean fixing up all demos/examples/test to compile
without error by either fixing the compiler to know about missing globals, or
by adding pragmas for globals we can't know about.
6. JavascriptGenerator#1390: We might want to emit a warning when we discover
and explicit `with` statement to say that it is deprecated (it is not permitted
in ES5 strict, and will probably go away completely in ES6)
Issues:
1. ClassModel.java#1010: We've tried pretty hard not to have any
platform-specific code in the tag compiler (the LZX-to-Javascript phase). Your
introducing an env.isDHTML here, and unless there are strong reasons for doing
so, I'd rather not see that.
2. NodeModel#838, #865, Seems to me extends, jsname, jsextends, should all fall
under the metamodel scheme and be skipped there, rather than called out as
literal constants.
3. SchemaBuilder/addEvents: This is a mess that I created that needs to be
cleaned up. See André's comment on
http://jira.openlaszlo.org/jira/browse/LPP-9675. At the very least, my idea to
auto-emit event declarations seems broken. Perhaps that should just be backed
out? I can't tell if your change relies on auto-event declaration, hopefully
not.
4. CommonGenerator: "prototype" is _not_ a property of Object instances, only
of Function instances (and hence of our Classes, which are implemented as
ECMAScript Functions. Function instances also have length, apply, and call as
properties.
5. CommonGenerator: We should probably add to the ECMA global properties the
non-standard (but nearly always present in our target runtimes) global object
properties (which can be found in ECMA-262-5 B.2: escape and unescape. This
would solve some of the issues you report in your test comments.
6. CommonGenerator: Similarly, we include in pre-defined globals `lz`, which
is defined early on by the runtime. It seems you should add `canvas` to this
list because it is an implicitly-defined global in all user programs, bound to
the single instance of the <canvas> tag.
7. JavascriptGenerator#1451: Would it be useful here, rather than clearing
possibleInstance, just remove the ones you have bound and then assert that it
is empty?
8. ScriptClass: I don't understand what is going on here. If you are renaming
these variables only here, it seems they must not be referenced in the first
place, so I wonder why we are emitting these. It seems there must be a deeper
issue that this is papering over?
9. Can we add some test cases to test/smoke/compiler.lzl? A particular test I
would like to see is a method with an inner function that references a global,
does/doesn't shadow an instance variable, etc.
Overall, this is looking good!
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
>
>
>
>
>
