tfpt review "/shelveset:RubyClrInterop06;REDMOND\curth" Comment : Create constructors on generated types that match each base class constructor Allocator logic not yet updated to use new constructors -- Curt Hagenlocher curth at microsoft.com -------------- next part -------------- A non-text attachment was scrubbed... Name: RubyClrInterop06.diff Type: application/octet-stream Size: 13338 bytes Desc: RubyClrInterop06.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20081124/5cf92b31/attachment.obj>
In DefineConstructors, newParams seems to be unnecessarily copied.
Wouldn''t it be better to do:
Type[] newParams;
Type[] baseParams = baseCtor.GetParameters();
if (has ruby class) {
newParams = baseParams;
} if (is serializer) {
newParams = baseParams;
} else {
...
newPrams = ArrayUtils.Insert(typeof(RubyClass), baseParams);
}
?
I would also split this method to multiple pieces, seems too big already.
Other than that, looks good!
Tomas
-----Original Message-----
From: Curt Hagenlocher
Sent: Monday, November 24, 2008 3:23 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: RubyClrInterop06
tfpt review "/shelveset:RubyClrInterop06;REDMOND\curth"
Comment :
Create constructors on generated types that match each base class constructor
Allocator logic not yet updated to use new constructors
--
Curt Hagenlocher
curth at microsoft.com
Unfortunately, GetParameters() doesn''t return a type array -- it
returns a ParameterInfo array. Here we see the evil of "var" -- it
hides the actual type :).
I''ll look for opportunities to split the method;
-----Original Message-----
From: Tomas Matousek
Sent: Monday, November 24, 2008 4:03 PM
To: Curt Hagenlocher; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: RubyClrInterop06
In DefineConstructors, newParams seems to be unnecessarily copied.
Wouldn''t it be better to do:
Type[] newParams;
Type[] baseParams = baseCtor.GetParameters();
if (has ruby class) {
newParams = baseParams;
} if (is serializer) {
newParams = baseParams;
} else {
...
newPrams = ArrayUtils.Insert(typeof(RubyClass), baseParams);
}
?
I would also split this method to multiple pieces, seems too big already.
Other than that, looks good!
Tomas
-----Original Message-----
From: Curt Hagenlocher
Sent: Monday, November 24, 2008 3:23 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: RubyClrInterop06
tfpt review "/shelveset:RubyClrInterop06;REDMOND\curth"
Comment :
Create constructors on generated types that match each base class constructor
Allocator logic not yet updated to use new constructors
--
Curt Hagenlocher
curth at microsoft.com
Alright then :)
-----Original Message-----
From: Curt Hagenlocher
Sent: Monday, November 24, 2008 5:46 PM
To: Tomas Matousek; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: RubyClrInterop06
Unfortunately, GetParameters() doesn''t return a type array -- it
returns a ParameterInfo array. Here we see the evil of "var" -- it
hides the actual type :).
I''ll look for opportunities to split the method;
-----Original Message-----
From: Tomas Matousek
Sent: Monday, November 24, 2008 4:03 PM
To: Curt Hagenlocher; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: RubyClrInterop06
In DefineConstructors, newParams seems to be unnecessarily copied.
Wouldn''t it be better to do:
Type[] newParams;
Type[] baseParams = baseCtor.GetParameters();
if (has ruby class) {
newParams = baseParams;
} if (is serializer) {
newParams = baseParams;
} else {
...
newPrams = ArrayUtils.Insert(typeof(RubyClass), baseParams);
}
?
I would also split this method to multiple pieces, seems too big already.
Other than that, looks good!
Tomas
-----Original Message-----
From: Curt Hagenlocher
Sent: Monday, November 24, 2008 3:23 PM
To: IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: Code Review: RubyClrInterop06
tfpt review "/shelveset:RubyClrInterop06;REDMOND\curth"
Comment :
Create constructors on generated types that match each base class constructor
Allocator logic not yet updated to use new constructors
--
Curt Hagenlocher
curth at microsoft.com