From: Christoph <chr_news@...>
Date: 2002-09-04T18:55:19+09:00
Subject: RE: Set.rb patch


Akinori MUSHA 
...
> 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.

Yes, the style thing is really stupid -  I actually forgot that the
patch contained these changes ... for white spaces I'll pass the 
bucket and blame it on my editor;-)

> 
> 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?

Your initialize method accepts a false argument 

  Set.new(false) == Set[]

Is this intentional?  In the loop detection you never back 
out ids from your ``already seen'' Set. This can cause
a false positive ``loop alarm''.  I'll try to answer 
the `` eql?, =='' question(s) in a second email.


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

I did add tests to all (perceived) buglets.

> 
>

...


> >  --- == 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

Sorry, a leftover from a first reading ... 

> 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)

The Comparable semantics recently changed. A ``nil'' return of  
   
   a <=> b 

signifies that a,b are incomparable, in other words

   a < b, a == b and b > a 

will return false - in particular no exception is raised - e.g. 
(with my modification) you'll find that

    p Set[1,2] < Set[2,3]  # false.

I also like to point out that sub(super)set relation is the 
prototypical  example for a ``partial order''. In fact, the new
Comparable semantics was probably introduced with classes like
Set in mind.   

> 
> > 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 })

Think about it.  The #contain? together with the #include?
method is probably the single most important Set method 
and it ought to be possible to express the efficient 
containment test for

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

using the Set Class. Admittedly the meaning

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

needs some time to get used to, however this is no
different to the various block options for the methods
of the other container classes. For example, I always
need to look up that 

 h = { "a" => 10, "b" => 20 }  
 h.delete("c") { |e| "prints #{e}" }  # prints c.

If the set class does not provide such a functionality
we will end up writing

    bar.all? {|x| foo.include?(e.upcase) }


in space or time critical places. Besides, the block
option is good place to distinguish contain? form 
<=> ;-)


> 
> 
> Thanks for all these suggestions, I much appreciate.

And thank you for writing this class - in particular for 
implementing the super-cool #divide, #print routines and
the many test cases.  I'll try to send a cleaned up patch
(without blocks) later today or tomorrow.


/Christoph