From: Akinori MUSHA <knu@...>
Date: 2002-09-04T12:44:40+09:00
Subject: Re: Set.rb patch

Hello,

I'd appreciate your patch, but next time would you please separate
real changes from irrelevant style fixes?  It is so tough for me to
read a patch which is a mixture of style fixes (whitespace adjustments
especially) and relevant changes.  I have my own taste in coding and
you have your own, so it is natural that they can conflict in some
places.  For example, I prefer obj.is_a?(Klass) to Klass === obj
because I think the former is more readable, but you may have your own
opinion.

However, I guess I could accept some/many of the improvements like
using all?, improvements on _flatten(), and so on.

> the included patch set.rb.diff resolves bugs in initialize,
> flatten(!), eql?

Would you elaborate?  Such bugs must be addressed and made detectable
by adding proper tests to the test suite.

>    def ==(set)
>      equal?(set) and return true
> -
> -    set.is_a?(type) && size == set.size or return false
> -
> -    set.each { |o| include?(o) or return false }
> -
> -    true
> +    unless Set === set and size == set.size
> +      false
> +    else
> +      all? { |e| set.include?(e) }
> +    end
>    end

Good catch.  It was my overlook to have missed a case like this:

	class SetWithOrder < Set
	  ...
	end

	class SetWithCondition < Set
	  ...
	end

	a = SetWithOrder.new([3,2,1])
	b = SetWithCondition.new(1..10) { |e| e <= 3 }

	a == b	# should be true

> +    def test_eql?
> +       a = EqlClass.new 
> +       b = EqlClass.new until b.hash == a.hash
> +       assert !(Set[a].eql?(Set[b]))
> +    end

This is not eql?() is expected to be.  The only requirement to eql?()
is make (a == b => a.eql?(b)) always true; in other words, guarantee
(!a.eql?(b) => a != b).  See how Hash compares keys: PTR_NOT_EQUAL()
in st.c.

In addition, it is also desirable that eql?() is not too a heavy
function to call frequently, so aliasing eql?() to ==() is not likely
to be a nice idea. (I'd doubt Set is suitable for Hash keys anyway,
though)

>  --- == set
>      Returns true if two sets are equal.  The equality of each couple
> -    of elements is defined according to Object#eql?.
> +    of elements is defined according to Object#==.

I'm afraid this change is wrong.  Note that it is saying about
comparison of elements.  As long as it uses Set#include?, comparison
is done using Object#eql?.

> +    Comparable
> +  def <=>(o)

Isn't it good enough to have ==() and contain?() (which does <=/=>) ?
I didn't mark Set as Comparable because I couldn't think of a case
where < and > are useful, when a > b raises an exception if a.<=>(b)
returns nil. (and your implementation does)

> and adds a block option to initialize and contain?.

I am not sure if it'd be obvious for initialize() and contain?() to
take an optional block.  It could be useful, but how the block is used
is not very obvious, to me at least.  I mean, I just can't grasp at a
glance what the following piece of code means:

	foo.contain?(bar) { |o| o.upcase }

It might be less efficient, but I'd recommend doing like this:

	foo.contain?(bar.map { |o| o.upcase })


Thanks for all these suggestions, I much appreciate.


Regards,

-- 
                     /
                    /__  __            Akinori.org / MUSHA.org
                   / )  )  ) )  /     FreeBSD.org / Ruby-lang.org
Akinori MUSHA aka / (_ /  ( (__(  @ iDaemons.org / and.or.jp

"When I leave I don't know what I'm hoping to find
              When I leave I don't know what I'm leaving behind.."