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 |
Free forum by Nabble | Edit this page |