[Ironruby-core] Code review: String#unpack, Bignum and Array related stuff

Tomas Matousek Tomas.Matousek at microsoft.com
Wed Jul 22 18:25:31 EDT 2009


Actually, wouldn't

public static IList/*!*/ Zip(CallSiteStorage<EachSite>/*!*/ each, ConversionStorage<IList>/*!*/ tryToAry, BlockParam block,  object self, [DefaultProtocol, NotNull, NotNullItems]params IList[] args) {

work? If [DefaultProtocol] is applied on a params array of IList it converts all parameters to IList via to_ary method.

Also Enumerable.Zip should be split into 2 methods: one that performs to_a conversion on all object args and other (internal) strongly typed method that takes IList[] and performs no conversions.
IList.zip should then call the latter so that to_a conversions are not performed unnecessarily on already converted values.

This is what I tested in MRI:

class C
  def respond_to? name
    puts name
    true
  end

  def to_ary
    [1]
  end

  def to_a
    [2]
  end
end

class D
  include Enumerable

  def each
    1
  end
end

[1,2,3].zip(C.new, C.new, C.new) #=> to_ary, to_ary, to_ary
D.new.zip(C.new, C.new, C.new) #=> to_a, to_a, to_a

Tomas

-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Shri Borde
Sent: Wednesday, July 22, 2009 2:52 PM
To: ironruby-core at rubyforge.org
Subject: Re: [Ironruby-core] Code review: String#unpack, Bignum and Array related stuff

Looks good. In Array#zip, you prefer casting to IList, and use Protocols.TryCastToArray as a fallback. Is that correct? If an IList object defines to_a, shouldn't to_a get called?

The array argument to Array#zip is marked as [NotNull]. Should it also be marked as [NotNullItems]?

Thanks,
Shri


-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Daniele Alessandri
Sent: Wednesday, July 22, 2009 1:51 PM
To: ironruby-core at rubyforge.org
Subject: Re: [Ironruby-core] Code review: String#unpack, Bignum and Array related stuff

Just a quick note: updated with
http://github.com/nrk/ironruby/commit/82feca718fc17ad45747ca45a1c92250abfcfce0

On Sun, Jul 19, 2009 at 11:29, Daniele Alessandri<suppakilla at gmail.com> wrote:
> Hi
> first of all I am trying a different approach to submitting my
> patches: I created a code_review branch on my repository in which I am
> going to push all my fixes for review. Fixes that will pass review
> will be rebased (not merged) in my main branch (ready to get pulled
> into the ironruby repository), the others will remain staged in my
> code_review branch for further works until they are "approved". I am
> just experimenting a different git workflow :-)
>
> Anyway, let's move on to the new commits:
>
>
> *
> http://github.com/nrk/ironruby/commit/48a4a02b5b47d61f2f7a3f3887ea4bf0
> 2d63edb4
>
> - More fixes for String#unpack:
>   o Fixed errors when unpacking data with 'Z' and '@' directives
>   o Implemented 'Q' and 'q' directives
>
>
> *
> http://github.com/nrk/ironruby/commit/17b5edab64967b0200370edf6657321a
> 0f79953d
>
> - Array related fixes:
>   o Array#== does not call #to_ary on its argument but it calls #to_a (it
>   basically performs a conversion of the argument rather than casting it to a
>   ruby array)
>   o Array#zip calls #to_ary to cast the argument to an Array, therefore it
>   overrides Enumerable#zip in which the argument is converted by calling
>   #to_a.
>
>
> *
> http://github.com/nrk/ironruby/commit/547d810c78b38afc34e728855ab7d0c2
> 0f499719
>
> - Bignum related fixes:
>   o Changed the signature for ClrBigInteger.ToString(self,radix), now
>   Bignum#to_s works as expected raising an ArgumentException if base is less
>   than 2 or higher than 36.
>   o Fixed Bignum#divmod, Bignum#remainder, Bignum#% and Bignum#modulo to work
>   with Float values as argument.
>   o Fixed Bignum#/ and Bignum#div as they behave differently with each other
>   when Float values are passed as argument.
>
> Note: Now bignum does not fail any of the expectations in core/bignum.
>
> See also the attached diff.
>
> Thanks,
> Daniele


--
Daniele Alessandri
http://www.clorophilla.net/blog/
http://twitter.com/JoL1hAHN
_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core
_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core


More information about the Ironruby-core mailing list