Forum: IronRuby More fixes for core/array specs (again)

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
1c29f9b1bf5f1b88ed8b0c9a9be39788?d=identicon&s=25 Daniele Alessandri (Guest)
on 2009-04-21 06:04
(Received via mailing list)
Attachment: core-array_fixes090420.diff (50 KB)
Hi,
I have attached a diff with new fixes for a whole bunch of failing
expectations in the core/array specs.

* Slightly modified IListOps.UniqueSelf as suggested upon review.
* Fixed IListOps.ReverseIndex as Array#rindex should not fail if
elements are removed from the array during the iteration over its
elements.
* Changed IListOps.Reverse to return IList instances of the same type
of the given self argument (this change also fixes the following
failing spec: "Array#reverse returns subclass instance on Array
subclasses")
* Changed IListOps.SetElement to make it try to invoke #to_ary on its
argument for multi-element sets.
* Changed one overload of IListOps.Equals to make it try to call
#to_ary on the object argument of Array#== and use the resulting array
as the actual argument for the equality check.
* Cleaning up tags removing expectations that do not fail anymore.
* Added ArrayOps.ToAry as Array#to_a and Array#to_ary behave
differently on subclasses of arrays.
* Various changes to IListOps.Join to clear all of the remaining tags
for the specs of Array#join. The tags marked as critical in
join_tags.txt are not related to pending bugs for Array#join.
* Changed IListOps.Repetition to return IList instances of the same
type of the given self argument (this fixes also "Array#* with an
integer returns subclass instance with Array subclasses")
* Modified IListOps.RecursiveJoin to make it flag the resulting string
as tainted if the given array, at least one of its elements or the
separator string are tainted.
* IListOps.Difference now uses Object#hash and Object#eql? to check
for object equality, this fixes the failing spec "Array#- acts as if
using an intermediate hash to collect values"

These changes directly clear the following failing expectations:

Array#[]= calls to_ary on its rhs argument for multi-element sets
Array#== calls to_ary on its argument
Array#join tries to convert the passed seperator to a String using
#to_str
Array#join checks whether the passed seperator responds to #to_str
Array#join handles recursive arrays
Array#join does not consider taint of either the array or the
separator when the array is empty
Array#join returns a string which would be infected with taint of the
array, its elements or the separator when the array is not empty
Array#join does not process the separator if the array is empty
Array#join raises a TypeError if the passed separator is not a string
and does not respond to #to_str
Array#- acts as if using an  intermediate hash to collect values
Array#* with an integer returns subclass instance with Array subclasses
Array#* with an integer copies the taint status of the original array
even if the array is empty
Array#* with an integer copies the taint status of the original array
if the passed count is not 0
Array#* with a string returns a string which would be infected with
taint of the array, its elements or the separator when the array is
not empty
Array#reverse returns subclass instance on Array subclasses
Array#rindex does not fail when removing elements from block

I have only a question: I've made changes to IListOps.Reverse and
IListOps.Repetition in order to make them return IList instead of
RubyArray. In ruby, Array#reverse and Array#* maintain the same type
of self for the returning object (that is, if self is a
RubyArray.Subclass then it is returned a new object of the same type).
With these changes we would actually be able to do something like
this:

>>> arraylist = System::Collections::ArrayList.new([1,2,3,4,5])
=> [1, 2, 3, 4, 5]
>>> multiplied_arraylist = arraylist * 2
=> [1, 2, 3, 4, 5, 1, 2, 3, 4, 5]
>>> multiplied_arraylist.class
=> System::Collections::ArrayList

Would it be a consistent behaviour?

On a (partially) unrelated note, I've just noticed that
List<T>.Reverse is more like Array#reverse! but it actually take the
precedence over IListOps.Reverse. I'm just thinking if this could lead
to some sort of confusion (ohan I'm not sure if anything can be done
about this.

Thanks,
Daniele


PS: I'm waiting to push these changes on my remote repository, Jim
told he is going to pull today but I would like to wait for a review
first (and yeah, today I'm too lazy to actually start off a new branch
:-)).
Aea6cfe04952626ab630bde47ff82f89?d=identicon&s=25 Shri Borde (Guest)
on 2009-04-21 06:58
(Received via mailing list)
Looks great!

Could you copy the comment "# MRI follows hashing semantics here, so
doesn't actually call eql?/hash for Fixnum/Symbol" from
core\array\minus_spec.rb to IListOps.Difference as its not obvious why
the code in IListOps.Difference does the complicated checks.

Yes, it seems OK that you can call all the Array methods on
System::Collections::ArrayList (and any System::Collectinos::IList
object in general). We are exposing Ruby String methods on
System::String, and it would be consistent with that approach.

About List<T>.Reverse, you could argue that if you use Ruby casing of
"reverse", IListOps should get precedence. Could you open a bug for this
please? If you use "Reverse", then the CLR method should get precedence,
and there would be a possibility of confusion, but as you say, there is
not much that can be done. It does not seem worth special-casing.

Thanks,
Shri
Ade8632553a9243ae05fc920f68644c1?d=identicon&s=25 Jim Deville (Guest)
on 2009-04-21 08:45
(Received via mailing list)
Daniele,

If you push this out tonight, I'll pull it in tomorrow.

JD
1c29f9b1bf5f1b88ed8b0c9a9be39788?d=identicon&s=25 Daniele Alessandri (Guest)
on 2009-04-21 13:46
(Received via mailing list)
Right now I don't have access to my local repository, I think to be
able to push on github by 9:00 PM CEST (UTC+2, that's my timezone, it
should be 12:00 PM PDT).

On Tue, Apr 21, 2009 at 08:39, Jim Deville <jdeville@microsoft.com>
wrote:
>> To: ironruby-core@rubyforge.org
>> System::Collections::ArrayList (and any System::Collectinos::IList
>> Thanks,
>> Hi,
>> subclasses")
>> join_tags.txt are not related to pending bugs for Array#join.
>> These changes directly clear the following failing expectations:
>> passed separator is not a string and does not respond to #to_str
>>
>> >>> multiplied_arraylist = arraylist * 2
>>
>> http://www.clorophilla.net/blog/
>
--
Daniele Alessandri
http://www.clorophilla.net/blog/
http://twitter.com/JoL1hAHN
1c29f9b1bf5f1b88ed8b0c9a9be39788?d=identicon&s=25 Daniele Alessandri (Guest)
on 2009-04-21 13:59
(Received via mailing list)
OK I'll add that comment to IListOps.Difference.
From what I've seen yesterday "reverse" and "Reverse" both call the
CLR method of List<T>, I'll try again later to confirm this behaviour
just to be really sure before I open a bug.

Thanks,
Daniele

On Tue, Apr 21, 2009 at 06:57, Shri Borde <Shri.Borde@microsoft.com>
wrote:
>
> * Slightly modified IListOps.UniqueSelf as suggested upon review.
> * IListOps.Difference now uses Object#hash and Object#eql? to check for object equality, 
this fixes the failing spec "Array#- acts as if using an intermediate hash to collect 
values"
> I have only a question: I've made changes to IListOps.Reverse and IListOps.Repetition in 
order to make them return IList instead of RubyArray. In ruby, Array#reverse and Array#* 
maintain the same type of self for the returning object (that is, if self is a 
RubyArray.Subclass then it is returned a new object of the same type).
> Would it be a consistent behaviour?
> Daniele Alessandri
> http://www.clorophilla.net/blog/
> http://twitter.com/JoL1hAHN
> _______________________________________________
> Ironruby-core mailing list
> Ironruby-core@rubyforge.org
> http://rubyforge.org/mailman/listinfo/ironruby-core
>



--
Daniele Alessandri
http://www.clorophilla.net/blog/
http://twitter.com/JoL1hAHN
1c29f9b1bf5f1b88ed8b0c9a9be39788?d=identicon&s=25 Daniele Alessandri (Guest)
on 2009-04-21 20:47
(Received via mailing list)
Just pushed my changes (with the added comment to IListOps.Difference
as suggested by Shri).

http://github.com/nrk/ironruby/commit/722dd4d2dc23...

Thanks,
Daniele


On Tue, Apr 21, 2009 at 13:43, Daniele Alessandri <suppakilla@gmail.com>
wrote:
>>
>>> doesn't actually call eql?/hash for Fixnum/Symbol" from
>>> this please? If you use "Reverse", then the CLR method should get
>>> bounces@rubyforge.org] On Behalf Of Daniele Alessandri
>>> elements are removed from the array during the iteration over its
>>> * Cleaning up tags removing expectations that do not fail anymore.
>>> separator string are tainted.
>>> arrays Array#join does not consider taint of either the array or the
>>> if the passed count is not 0
>>> With these changes we would actually be able to do something like
>>>
>>> he is going to pull today but I would like to wait for a review first
>>
> http://www.clorophilla.net/blog/
> http://twitter.com/JoL1hAHN
>



--
Daniele Alessandri
http://www.clorophilla.net/blog/
http://twitter.com/JoL1hAHN
Ade8632553a9243ae05fc920f68644c1?d=identicon&s=25 Jim Deville (Guest)
on 2009-04-22 06:05
(Received via mailing list)
Awesome. I just started putting the other set of changes (Shri, Daniele
and Jirapong's changes as of ~7pm yesterday) through the continuous
integration troll we call SNAP.

I'll start pulling in the next set right away (Expected to be this
change, my changes, and Jimmy's changes)

JD
This topic is locked and can not be replied to.