DependentsArray size how does it work ?

DependentsArray>>size
 ^self inject: 0 into: [ :count :dep | dep ifNotNil: [ count := count + 1 ]]

Byte Code:
9 <70> self
10 <75> pushConstant: 0
11 <89> pushThisContext:
12 <77> pushConstant: 2
13 <C8> send: blockCopy:
14 <A4 0E> jumpTo: 30
16 <69> popIntoTemp: 1
17 <68> popIntoTemp: 0
18 <11> pushTemp: 1
19 <73> pushConstant: nil
20 <C6> send: ==
21 <99> jumpFalse: 24
22 <73> pushConstant: nil
23 <94> jumpTo: 29
24 <10> pushTemp: 0
25 <76> pushConstant: 1
26 <B0> send: +
27 <81 40> storeIntoTemp: 0
29 <7D> blockReturn
30 <F0> send: inject:into:
31 <7C> returnTop

inject: thisValue into: binaryBlock
 | nextValue |
 nextValue := thisValue.
 self do: [:each | nextValue := binaryBlock value: nextValue value: each].
 ^nextValue

"This one works..."
| obj  |
obj := Object new.
obj addDependent: nil.
obj addDependent: $0.
obj addDependent: nil.
^obj dependents size


It seems that i can learn something with this one:

1) in squeak you can overwrite block arguments (storeIntoTemp: 0), in not many
Smalltalk you can do that i guess (not in VW anyway).
In my opinion this is never something to do.

2) there must be a trick somewhere...

On first pass, inject:into: should evaluate with count:0 dep:nil
We will go through:
22 <73> pushConstant: nil
23 <94> jumpTo: 29
...
29 <7D> blockReturn

Then the block should answer nil, shouldn't it ?

Then, on second pass, shouldn't the block evaluate with: count:nil dep:$0 ?
Then, we would have count := nil+1 that would fail, that's what i imagined...

Not at all, all is going fine. Can anybody teach me ?

On 08.02.2006, at 04:38, nicolas cellier wrote:

> 1) in squeak you can overwrite block arguments (storeIntoTemp: 0),  
> in not many
> Smalltalk you can do that i guess (not in VW anyway).
> In my opinion this is never something to do.

Yes, evil.

> 2) there must be a trick somewhere...

The do: is reimplemented, too. And inject:into: uses do:.
The do: now filters for nil, thus the count:0 dep:nil will not execute
anything.

And as dep can never be nil, the method could, I think,  look like this:

size
        ^self inject: 0 into: [ :count :dep |  count + 1 ]


     Marcus

Le Mercredi 08 Février 2006 09:46, vous avez écrit :
> On 08.02.2006, at 04:38, nicolas cellier wrote:
> > 1) in squeak you can overwrite block arguments (storeIntoTemp: 0),
> > in not many
> > Smalltalk you can do that i guess (not in VW anyway).
> > In my opinion this is never something to do.
>
> Yes, evil.
>
> > 2) there must be a trick somewhere...
...
Thank you Marcus. A good lesson to me.

I did not search in the right place, sticking to this ifNotNil: that prevented
me to go further...

> And as dep can never be nil, the method could, I think,  look like this:
>
> size
>  ^self inject: 0 into: [ :count :dep |  count + 1 ]
>
>      Marcus

Waouh, very nice. But i am afraid i would not understand such a code neither
Pretending i do not know of the do: re-implementation trick, i would not see
why this size should be different from super size.

I did not understand how first implementation did work, but i understood
programmer's intention.
I used this example to propose a clean understandable collection extension:
    ^self count: [:e | e notNil]

What you're proposing above sounds a little bit like:
    ^self count: [:e | true]

Apart writing the block argument, all code above is correct, tricky but
correct.
At least, there is a programmer that can be proud driving me nuts!
Tricky code deserve either refactoring or implementation notes i think.

By the way, is the #at: message also redefined ? (did not check yet).
If not, following (bad style) code could be broken with DependentsArray :
    1 to: someDependents size do: [:i | (someDependents at: i) doSomething]

I will try it this evening. if not redefined, then the whole class seems
dangerous to me and there might be other side effects.

On 08.02.2006, at 13:58, nicolas cellier wrote:


But of what help is that here? You would need to explain this, too.

> Apart writing the block argument, all code above is correct, tricky  
> but
> correct.
> At least, there is a programmer that can be proud driving me nuts!

You will find *a lot* of code of that quality in Squeak... part of  
that is
because Smalltalk is older than the style of programming that was  
invented
after people used it for some decades... and because there were other  
contraints
on the Alto than on today's machines (memory, performance, code size...)

And then there is a lot of stuff that's just bad design... e.g. done  
in a hurry because
you don't win with style points when you need to do finish a demo for  
next week.

The real question is, in general, how to re-engineer, improve and  
evolve such a
systems without breaking it, and especially without breaking the  
clients that use it.

Huge problem...

> Tricky code deserve either refactoring or implementation notes i  
> think.
>

indeed.

>
> I will try it this evening. if not redefined, then the whole class  
> seems
> dangerous to me and there might be other side effects.
>

It gets interesting when you look at the real users of DependArray. The
only place is

Array>>copyWithDependent: newElement
        self size = 0 ifTrue:[^DependentsArray with: newElement].
        ^self copyWith: newElement


not directly obvious way of doing it, either: lazy convertion of empty
arrays, with the place where the real instanciation should have  
happended
nicely hidden...

        Marcus

Le Mercredi 08 Février 2006 14:32, vous avez écrit :
Good evening Marcus,

I will try to focus on real bugs rather than theoretical ones if i can.

I am simply browsing the 33 senders of #dependents  in a 3.8 image.
they will use #myDependents wich will use DependentsArray.

And after 15 minutes, i think i have a clue for potential bug.

Object>>hasUnacceptedEdits
    "Answer true if any of the views on this object has unaccepted edits."

    self dependents
        do: [:each | each hasUnacceptedEdits ifTrue: [^ true]]
        without: self.
    ^ false

does call #do:without: wich is not implemented with a #do: loop but rather (1
to: self size do:).

SequenceableCollection>>do: aBlock without: anItem
    "Enumerate all elements in the receiver.
    Execute aBlock for those elements that are not equal to the given item"
    "Refer to the comment in Collection|do:."
    1 to: self size do:
        [:index | anItem = (self at: index) ifFalse:[aBlock value: (self at:
index)]]

If one ever nil a slot (remove a dependent), then there will be harmless
effects in appearance because (nil hasUnacceptedEdits) will fall back in
Object>>hasUnacceptedEdits and return false without entering an infinite loop
since nil has no dependent.

But then, execution will stop at self size, and some real dependents
won't have a chance to tell it hasUnacceptedEdits...

I do not know what are the consequences (a window might close without saving?)
and i can not exhibit a failure case so the bug is still theoretical.
Maybe there are not many cases when you simply remove a dependent without
adding a new one, and adding a new one will create a copy of myDependents, in
this case the bug won't occur, because copy eliminate the nil.

So do you see the possible side effects i were speaking of ?

We should be able to construct a failing code.
Maybe have to search buglist and mailing list to check if someone ever
experienced something similar...

The same potential problem exist with these ones:

SystemWindow>>replacePane: oldPane with: newPane
    "Make newPane exactly occupy the position and extent of oldPane"

    | aLayoutFrame hadDep |
    hadDep := model dependents includes: oldPane.
etc...

because includes will fall back to (1 to: self size do:) again...

and also in:
TestRunner>>installProgressWatcher
    | win host |
    win := self dependents first.
    ...
This one should rather open an exception i think.

What is the cure ?
I do not know, losely implement in DependentsArray all these collection
messages that are implemented using (1 to: self size do:) above.
At least #do:without: , #indexOf:startingAt:ifAbsent:, #first, maybe more...

If someone ever change some implementation in a collection superclass (i do
not know why, efficiency ?), or some one use a new message to act on
dependents, then same problem will arise again and will be hard to discover
and to fix (if it silently close your window without exception...).

We should find a better fix than patch i propose above...

One more question on dependents: how will DependentsArray have nil elements ?

Not via the #removeDependent: message that would call #reject: and answer a
copy without the nils.

Apart if you add explicitly nil (or implicitly via a variable) in dependents,
i do not see yet how there would be a nil.

In this case, there is just some useless code, and the bug i did figure will
never show up.
But i may miss something...

From: "nicolas cellier" <[hidden email]>


> One more question on dependents: how will DependentsArray have nil
elements ?
DependentArray is a weak subclass; you can see that in the class
definition:
Array weakSubclass: #DependentsArray
     instanceVariableNames: ''
     classVariableNames: ''
     poolDictionaries: ''
     category: 'Kernel-Objects'

An element of a weak array is automatically replaced with nil when it
becomes unreachable.This is one of the services of the garbage
collector.

Greetings
Boris

Le Mercredi 08 Février 2006 22:58, vous avez écrit :
Thanks Boris.

So the bug will eventually show up.

Unless one cautiously calls (model removeDependent: self).
But why bother when you know that you'll be automatically nilled...

Should i go to mantis with only a clue ?

sure

On 8 févr. 06, at 23:05, nicolas cellier wrote:

Le Jeudi 09 Février 2006 09:36, stéphane ducasse a écrit
> On 8 févr. 06, at 23:05, nicolas cellier wrote:
> >
> > Should i go to mantis with only a clue ?

> sure

OK, bug is on mantis with a test case and a first element of patch (not a
satisfying patch, just a work around), but it took me almost 2h for such a
small thing... I'm getting old i'm afraid !

Le Vendredi 10 Février 2006 09:03, stéphane ducasse a écrit :
> can you give us the mantis number?
>
Sure stef,

it is 2701 (http://bugs.impara.de/view.php?id=2701)