Re: Issue 2230 in pharo: ArrayedCollection class>>newFromStream:

Updates:
        Cc: [hidden email]

Comment #6 on issue 2230 by marianopeck: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

 From my point of view, it is really easy to change, and the correct form is:

self isBits | self isWords ifFalse: [^ super newFromStream: s].

what do you think?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Cc: [hidden email]

Comment #7 on issue 2230 by marianopeck: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

Any collectiions hacker agree with me?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #8 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

mariano did you check if the code did not change in squeak?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #9 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

By reading the rest of the code, ^s nextWordsInto:, I understand that the  
intention is to read a variableWord instance. So the guard self isWords  
ifTrue [^self] sounds very weird indeed. Let's simplify the logical  
expression step by step:
(self isPointers or: [self isWords]) not
(self isPointers not and: [self isWords not])
(self isBits and: [self isWords not])
(self isBits and: [self isBytes])
(self isBytes)

So the whole method is here to decode a variableWord instance which has  
been encoded in a stream of bytes, into a variableByte instance.
If it was a stream of words (32 bits) instead of a stream of bytes, my bet  
is that the method would not work neither, but there is no guard against  
this one, probably because we never encountered streams of words in our  
way ;).

There is more, (nextInt32) does not seem to handle endianness, while  
nextWordsInto: does...

It seems that the test (s next = 16r80) expects the most significant byte  
to come first in the stream (a size >= 2GiBytes is interpreted as  
compressed data).

 From what I see here, the construction seems fragile to me. If you really  
want to correct this stuff, you can't just polish superficial mud, you have  
to dig deeper into it, and understand the real intention. And maybe you'll  
have to backtrack in Squeak history... Maybe the best way to correct this  
is to help Stephane making its Ring-based history browser :)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #10 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

By the way, newFromStream: is really an usurpation of a generic name for  
something absolutely specific. We could try and name it accurately :  
newVariableByteInstanceConvertedFromVariableWordInstanceEncodedOnAByteStream:  
As you can smell, it stinks.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #11 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

In Squeak the code is:
(self isPointers or: [ self isWords not ]) ifTrue: [ ^self ].
note that returning self instead of failing sounds weird...

And before that, it was:
self isPointers | self isWords not ifTrue: [^ super newFromStream: s].
                "super may cause an error, but will not be called."

In this case, the length is given in number of words...
(WordArray basicNew: 4) byteSize -> 16

isWords means there is a variable number of slots (indexed) of 32bits, that  
could be either pointers (to other objects) or immediate 32bits integer  
values (positive, 0 to 16rFFFFFFFF).

So forget my previous analysis, it was guided by an incorrect version  
(yeah, we badly need an history browser !).


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #12 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

so :)

now I'm too tired and confused but the method name was great.



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #13 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

So basically, since in Pharo we still use the old version with | and proper  
not, we could change | to ( or: [])

Or not, since it's not exactly the worst overhead compared to what else  
this method will usually end up doing.

Alternatively, delete the method altogether, and create a TWordLike  
containing this, and other methods specific to word-like objects instead of  
having the methods with special checks in a superclass.
Might be too much to remember to include it when you variableWordSubclass:  
something though. :/


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: Workneeded

Comment #14 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixToInclude

Comment #15 on issue 2230 by marianopeck: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

Slice in inbox.



I finally put:


        self isBits | self isWords ifFalse: [
                self error: 'This method is only meant for raw bits and word-like  
subclasses '].


which I think is the most intention revealing way.


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Labels: Milestone-1.4

Comment #16 on issue 2230 by marianopeck: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #17 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

Mariano, you changed the logic !
The correct transform would be:

(isPointers | isWords not) ifTrue:
(isBits not | isWords not) ifTrue:
(isBits & isWords) not ifTrue:
(isBits & isWords) ifFalse:


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #18 on issue 2230 by marianopeck: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

Hi Nicolas. Maybe I have changed, maybe I am slept.... but, let's agree  
first on what we want:  this method should only be valid if the type is or  
bytes or words. Hence the method comment "Only meant for my subclasses  
that are raw bits and word-like.  For quick unpack form the disk."

So....why

self isBits | self isWords ifFalse: [
                self error: 'This method is only meant for raw bits and word-like  
subclasses '].


is incorrect?


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #19 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

Because the method only handles words, not bytes, see:

s nextWordsInto: ...


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #20 on issue 2230 by marianopeck: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

It's true. You are right. So, why the method comment is stil like that ?  
should we remove the part that says "raw bits" ?  and then why we cannot  
just simple have:

self isWords ifFalse: [
                self error: 'This method is only meant for raw bits and word-like  
subclasses '].

?

Thanks Nicolas for reviewing my change :)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #21 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

Ah, I see, you understood
- my subclasses that are raw bits
- and my subclass that are word like

But the intention was:
- my subclasses that are BOTH raw bits AND word like

Does it make sense? (this is really ambiguous, at least for us, latin  
fellows)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #22 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

isWords is not enough, because pointers are words too, thus it was

(isPointers | isWords not) ifTrue:
(isPointers not & isWords) ifFalse: ,

but I prefer (isBits & isWords) ifFalse: which guard exactly what the  
comment tells


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: Workneeded

Comment #23 on issue 2230 by [hidden email]: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Comment #24 on issue 2230 by marianopeck: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

In inbox it is ready: Name:  
SLICE-Issue-2230-ArrayedCollection-classnewFromStream-MarianoMartinezPeck.2  
(NOTICE THE 2 AT THE END).

THanks Nicolas. Indded, (isPointers not & isWords) ifFalse:
is the one I wanted to use. Now...

1) Yes, I am an idiot because I already knew this method was only for  
word-like objects, so I was blind by the method comment

2) I don't like #isWords at all. I thought it had the same semantics as  
#typeOfClass  where #words is answered for only those classes that has been  
created with the #variableWordSubclass...  So, I was expecting #isWords to  
answer true only for those as well. But it seems it is as you said,  
#isWords is also true for pointers...   I am not sure if I like this :(



_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker

Updates:
        Status: FixToInclude

Comment #25 on issue 2230 by marianopeck: ArrayedCollection  
class>>newFromStream:
http://code.google.com/p/pharo/issues/detail?id=2230

(No comment was entered for this change.)


_______________________________________________
Pharo-bugtracker mailing list
[hidden email]
http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-bugtracker