Refactored ClassBuilder.buildMembers (dynamic prop parse context)

The parser methods are now split into their own functions. This has a number
of benefits: The most immediate is the commit that will follow. The second
benefit is that the function is no longer a closure---all context
information is passed into it, and so it can be optimized by the JavaScript
engine accordingly.

lib/ClassBuilder.js

@@ -28,6 +28,9 @@ var util    = require( __dirname + '/util' ),
     warn    = require( __dirname + '/warn' ),
     Warning = warn.Warning,
 
+    hasOwn = Object.prototype.hasOwnProperty,
+
+
     /**
      * IE contains a nasty enumeration "bug" (poor implementation) that makes
      * toString unenumerable. This means that, if you do obj.toString = foo,
@@ -462,28 +465,44 @@ exports.prototype.buildMembers = function buildMembers(
     props, class_id, base, prop_init, memberdest, staticInstLookup
 )
 {
-    var hasOwn = Array.prototype.hasOwnProperty,
+    var context = {
-        defs   = {},
+        _cb: this,
+
+        // arguments
+        prop_init:        prop_init,
+        class_id:         class_id,
+        base:             base,
+        staticInstLookup: staticInstLookup,
+
+        defs: {},
+
+        // holds member builder state
+        state: {},
 
         // TODO: there does not seem to be tests for these guys; perhaps
         // this can be rectified with the reflection implementation
-        members          = memberdest.all,
+        members:          memberdest.all,
-        abstract_methods = memberdest['abstract'],
+        abstract_methods: memberdest['abstract'],
-        static_members   = memberdest['static'],
+        static_members:   memberdest['static'],
-        virtual_members  = memberdest['virtual'],
+        virtual_members:  memberdest['virtual'],
-
+    };
-        smethods = static_members.methods,
-        sprops   = static_members.props,
-
-        // holds member builder state
-        state = {},
-
-        _self = this
-    ;
 
     util.propParse( props, {
-        each: function( name, value, keywords )
+        each:     _parseEach,
+        property: _parseProp,
+        getset:   _parseGetSet,
+        method:   _parseMethod,
+    }, context );
+
+    // process accumulated member state
+    this._memberBuilder.end( context.state );
+}
+
+
+function _parseEach( name, value, keywords )
 {
+    var defs = this.defs;
+
     // disallow use of our internal __initProps() method
     if ( reserved_members[ name ] === true )
     {
@@ -505,39 +524,50 @@ exports.prototype.buildMembers = function buildMembers(
     // keep track of the definitions (only during class declaration)
     // to catch duplicates
     defs[ name ] = keywords;
-        },
+}
 
-        property: function( name, value, keywords )
+
+function _parseProp( name, value, keywords )
 {
-            var dest = ( keywordStatic( keywords ) ) ? sprops : prop_init;
+    var dest = ( keywordStatic( keywords ) )
+        ? this.static_members.props
+        : this.prop_init;
 
     // build a new property, passing in the other members to compare
     // against for preventing nonsensical overrides
-            _self._memberBuilder.buildProp(
+    this._cb._memberBuilder.buildProp(
-                dest, null, name, value, keywords, base
+        dest, null, name, value, keywords, this.base
     );
-        },
+}
 
-        getset: function( name, get, set, keywords )
+
+function _parseGetSet( name, get, set, keywords )
 {
-            var dest       = ( keywordStatic( keywords ) ) ? smethods : members,
+    var dest = ( keywordStatic( keywords ) )
+            ? this.static_members.methods
+            : this.members,
+
         is_static  = keywordStatic( keywords ),
         instLookup = ( ( is_static )
-                    ? staticInstLookup
+            ? this.staticInstLookup
             : exports.getMethodInstance
         );
 
-            _self._memberBuilder.buildGetterSetter(
+    this._cb._memberBuilder.buildGetterSetter(
-                dest, null, name, get, set, keywords, instLookup, class_id, base
+        dest, null, name, get, set, keywords, instLookup,
+        this.class_id, this.base
     );
-        },
+}
 
-        method: function( name, func, is_abstract, keywords )
+
+function _parseMethod( name, func, is_abstract, keywords )
 {
     var is_static  = keywordStatic( keywords ),
-                dest       = ( is_static ) ? smethods : members,
+        dest       = ( is_static )
+            ? this.static_members.methods
+            : this.members,
         instLookup = ( is_static )
-                    ? staticInstLookup
+            ? this.staticInstLookup
             : exports.getMethodInstance
     ;
 
@@ -552,9 +582,9 @@ exports.prototype.buildMembers = function buildMembers(
         }
     }
 
-            var used = _self._memberBuilder.buildMethod(
+    var used = this._cb._memberBuilder.buildMethod(
         dest, null, name, func, keywords, instLookup,
-                class_id, base, state
+        this.class_id, this.base, this.state
     );
 
     // do nothing more if we didn't end up using this definition
@@ -569,28 +599,23 @@ exports.prototype.buildMembers = function buildMembers(
     // smae name has already been seen
     if ( is_abstract )
     {
-                abstract_methods[ name ] = true;
+        this.abstract_methods[ name ] = true;
-                abstract_methods.__length++;
+        this.abstract_methods.__length++;
     }
-            else if ( ( hasOwn.call( abstract_methods, name ) )
+    else if ( ( hasOwn.call( this.abstract_methods, name ) )
         && ( is_abstract === false )
     )
     {
         // if this was a concrete method, then it should no longer
         // be marked as abstract
-                delete abstract_methods[ name ];
+        delete this.abstract_methods[ name ];
-                abstract_methods.__length--;
+        this.abstract_methods.__length--;
     }
 
     if ( keywords['virtual'] )
     {
-                virtual_members[ name ] = true;
+        this.virtual_members[ name ] = true;
     }
-        },
-    } );
-
-    // process accumulated member state
-    this._memberBuilder.end( state );
 }
 
 
@@ -741,9 +766,7 @@ exports.prototype.createConcreteCtor = function( cname, members )
         // Provide a more intuitive string representation of the class
         // instance. If a toString() method was already supplied for us,
         // use that one instead.
-        if ( !( Object.prototype.hasOwnProperty.call(
+        if ( !( hasOwn.call( members[ 'public' ], 'toString' ) ) )
-            members[ 'public' ], 'toString'
-        ) ) )
         {
             // use __toString if available (see enum_bug), otherwise use
             // our own defaults
@@ -991,8 +1014,7 @@ function attachStatic( ctor, members, base, inheriting )
             // we use hasOwnProperty to ensure that undefined values will not
             // cause us to continue checking the parent, thereby potentially
             // failing to set perfectly legal values
-            var has   = Object.prototype.hasOwnProperty,
+            var found = false,
-                found = false,
 
                 // Determine if we were invoked in the context of a class. If
                 // so, use that.  Otherwise, use ourself.
@@ -1011,16 +1033,16 @@ function attachStatic( ctor, members, base, inheriting )
             // available and we are internal (within a method), we can move on
             // to check other levels of visibility. `found` will contain the
             // visibility level the property was found in, or false.
-            found = has.call( props[ 'public' ], prop ) && 'public';
+            found = hasOwn.call( props[ 'public' ], prop ) && 'public';
             if ( !found && _self._spropInternal )
             {
                 // Check for protected/private. We only check for private
                 // properties if we are not currently checking the properties of
                 // a subtype. This works because the context is passed to each
                 // recursive call.
-                found = has.call( props[ 'protected' ], prop ) && 'protected'
+                found = hasOwn.call( props[ 'protected' ], prop ) && 'protected'
                     || !in_subtype
-                        && has.call( props[ 'private' ], prop ) && 'private'
+                        && hasOwn.call( props[ 'private' ], prop ) && 'private'
                 ;
             }
 

lib/util.js

@@ -257,7 +257,7 @@ exports.copyTo = function( dest, src, deep )
  *
  * @return undefined
  */
-exports.propParse = function( data, options )
+exports.propParse = function( data, options, context )
 {
     // todo: profile; function calls are more expensive than if statements, so
     // it's probably a better idea not to use fvoid
@@ -327,13 +327,13 @@ exports.propParse = function( data, options )
         // if an 'each' callback was provided, pass the data before parsing it
         if ( callbackEach )
         {
-            callbackEach.call( callbackEach, name, value, keywords );
+            callbackEach.call( context, name, value, keywords );
         }
 
         // getter/setter
         if ( getter || setter )
         {
-            callbackGetSet.call( callbackGetSet,
+            callbackGetSet.call( context,
                 name, getter, setter, keywords
             );
         }
@@ -341,7 +341,7 @@ exports.propParse = function( data, options )
         else if ( ( typeof value === 'function' ) || ( keywords[ 'proxy' ] ) )
         {
             callbackMethod.call(
-                callbackMethod,
+                context,
                 name,
                 value,
                 exports.isAbstractMethod( value ),
@@ -351,7 +351,7 @@ exports.propParse = function( data, options )
         // simple property
         else
         {
-            callbackProp.call( callbackProp, name, value, keywords );
+            callbackProp.call( context, name, value, keywords );
         }
     }
 };

test/Util/PropParseTest.js

@@ -210,4 +210,46 @@ require( 'common' ).testCase(
             propParse( { 'abstract foo': [ 'valid_name' ] }, {} );
         }, SyntaxError );
     },
+
+
+    /**
+     * The motivation behind this feature is to reduce the number of closures
+     * necessary to perform a particular task: this allows binding `this' of the
+     * handler to a custom context.
+     */
+    'Supports dynamic context to handlers': function()
+    {
+        var _self   = this;
+            context = {};
+
+        // should trigger all of the handlers
+        var all = {
+            prop:   'prop',
+            method: function() {},
+        };
+
+        // run test on getters/setters only if supported by the environment
+        if ( this.hasGetSet )
+        {
+            Object.defineProperty( all, 'getset', {
+                get: ( get = function () {} ),
+                set: ( set = function () {} ),
+
+                enumerable: true,
+            } );
+        }
+
+        function _chk()
+        {
+            _self.assertStrictEqual( this, context );
+        }
+
+        // check each supported handler for conformance
+        this.Sut.propParse( all, {
+            each:     _chk,
+            property: _chk,
+            getset:   _chk,
+            method:   _chk,
+        }, context );
+    },
 } );