Smalltalk › Squeak › Squeak - Dev

OrderedCollection>>size fails in debugger

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

12 messages Options

Ben Coman

OrderedCollection>>size fails in debugger

I am writing some code that uses OrderedCollection and was tracing
through it with the debugger when the debugger itself threw a DNU
exception. I narrowed down reproduction of this to executing the
following line, clicking <Debug> then eight times <Into> :
> self halt. OrderedCollection new: 5.

As far as I can determine, the root cause is that
"OrderedCollectionInspector>>fieldList" calls "object size" which has
the following implementation:
> OrderedCollection>>size
> ^ lastIndex - firstIndex + 1

where "lastIndex" is nil (and also "firstIndex" is nil).
To narrow it down further, the error can also be reproduced by executing
the following:
> OrderedCollectionInspector openOn: OrderedCollection basicNew.

I fixed this with:
> OrderedCollection>>size
> lastIndex ifNil: [ ^0 ].
> ^ lastIndex - firstIndex + 1

It would be more correct to also have included "firstIndex ifNil: [ ^0
]" but I assume both conditions occur at the same time, so I left it out
for performance.

Not knowing how performance issue are balanced in Squeak, I was
concerned that the "size" method might be called very often, and that
even this one extra line might want to be avoided - I assume that this
case only occurs in debugging at times when the object has not been
fully initialized, and this error would not occur from application
code. So I went looking for an alternative. Back in the caller, rather
than:
> OrderedCollection>>fieldList
> object ifNil: [ ^ OrderedCollection new].
> ^ self baseFieldList ,
> (object size <= (self i1 + self i2)
<snip>

this corrected the problem:
> OrderedCollection>>fieldList
> object ifNil: [ ^ OrderedCollection new].
> [ object size ] on: Exception do: [ ^ self baseFieldList ].
> ^ self baseFieldList ,
> (object size <= (self i1 + self i2)
<snip>
I assume this would also be required for
OrderedCollection>>selectedObjectIndex.

However now in the debugger inst-var sub-pane, selecting self shows
"<error in printString: evaluate "self printString" to debug>, and in so
doing reveals that the "self size" call fails in:
> OrderedCollection>>do: elementBlock separatedBy: separatorBlock
> 1 to: self size do:
indicating a similar fix is potentially required for the following methods:
OrderedCollection>>add: newObject afterIndex: index
OrderedCollection>>add: newObject beforeIndex: index
OrderedCollection>>at: index ifAbsentPut: block
OrderedCollection>>collect: aBlock
OrderedCollection>>copyReplaceFrom: start to: stop with:
replacementCollection
OrderedCollection>>makeRoomAtFirst
OrderedCollection>>makeRoomAtLast
OrderedCollection>>postCopyFrom: startIndex to: endIndex
OrderedCollection>>with: otherCollection collect: twoArgBlock
OrderedCollection>>withIndexCollect: elementAndIndexBlock
which is not as clean as modifying OrderedCollection>>size, and still a
bit uncertain.

What is the community's thoughts on this?

Finally, if any of these are desirable fixes, I would appreciate using
my first contribution as a practice for submitting to the Inbox (if
someone could direct me to some documentation that describes how to do
this.)

cheers, Ben

Frank Shearar-3

Re: OrderedCollection>>size fails in debugger

On 10 July 2011 17:16, Ben Coman <[hidden email]> wrote:
>
> Finally, if any of these are desirable fixes, I would appreciate using my
> first contribution as a practice for submitting to the Inbox (if someone
> could direct me to some documentation that describes how to do this.)
>
> cheers, Ben

Submission, at least, is easy:
* Update your (clean/virgin) Squeak trunk image.
* Make your changes.
* Open up a Monticello Browser (under the Tools menu at the top of
your Squeak window).
* Select the package/s you edited in the left pane. This/they will
have a "* " prefix. In this case, it's likely to be Collections.
* Select the http://source.squeak.org/inbox repository in the right
pane. Hit Save, and put in a decent commit message. I usually first
save to my local package-cache and Copy to the inbox; I'm fairly
paranoid about uploads because my ADSL connection's rubbish, and also
often work offline.

frank

Bert Freudenberg

Re: OrderedCollection>>size fails in debugger

On 10.07.2011, at 19:15, Frank Shearar wrote:

> * I usually first
> save to my local package-cache and Copy to the inbox; I'm fairly
> paranoid about uploads because my ADSL connection's rubbish, and also
> often work offline.

If you save to an http repo, MC will automatically save a copy in the package-cache first.

- Bert -

Frank Shearar-3

Re: OrderedCollection>>size fails in debugger

On 10 July 2011 18:25, Bert Freudenberg <[hidden email]> wrote:

>
> On 10.07.2011, at 19:15, Frank Shearar wrote:
>
>> * I usually first
>> save to my local package-cache and Copy to the inbox; I'm fairly
>> paranoid about uploads because my ADSL connection's rubbish, and also
>> often work offline.
>
> If you save to an http repo, MC will automatically save a copy in the package-cache first.
>
> - Bert -

Ah, handy to know!

frank

Levente Uzonyi-2

Re: OrderedCollection>>size fails in debugger

In reply to this post by Ben Coman

On Mon, 11 Jul 2011, Ben Coman wrote:

> I am writing some code that uses OrderedCollection and was tracing through it
> with the debugger when the debugger itself threw a DNU exception. I narrowed
> down reproduction of this to executing the following line, clicking <Debug>
> then eight times <Into> :
>> self halt. OrderedCollection new: 5.
>
> As far as I can determine, the root cause is that
> "OrderedCollectionInspector>>fieldList" calls "object size" which has the
> following implementation:
>> OrderedCollection>>size
>> ^ lastIndex - firstIndex + 1
>
> where "lastIndex" is nil (and also "firstIndex" is nil).
> To narrow it down further, the error can also be reproduced by executing the
> following:
>> OrderedCollectionInspector openOn: OrderedCollection basicNew.
>
> I fixed this with:
>> OrderedCollection>>size
>> lastIndex ifNil: [ ^0 ].
>> ^ lastIndex - firstIndex + 1
>
> It would be more correct to also have included "firstIndex ifNil: [ ^0 ]" but
> I assume both conditions occur at the same time, so I left it out for
> performance.
> Not knowing how performance issue are balanced in Squeak, I was concerned
> that the "size" method might be called very often, and that even this one
> extra line might want to be avoided - I assume that this case only occurs in
> debugging at times when the object has not been fully initialized, and this
> error would not occur from application code. So I went looking for an

Right.

> alternative. Back in the caller, rather than:
>> OrderedCollection>>fieldList
>> object ifNil: [ ^ OrderedCollection new].
>> ^ self baseFieldList ,
>> (object size <= (self i1 + self i2)
> <snip>
>
> this corrected the problem:
>> OrderedCollection>>fieldList
>> object ifNil: [ ^ OrderedCollection new].
>> [ object size ] on: Exception do: [ ^ self baseFieldList ].
>> ^ self baseFieldList ,
>> (object size <= (self i1 + self i2)
> <snip>
> I assume this would also be required for
> OrderedCollection>>selectedObjectIndex.

Yes, OrderedCollectionInspector should be changed, because that's what
causes the problem. I'd simply create an #objectSize method for it and use
that instead of object size.

> However now in the debugger inst-var sub-pane, selecting self shows "<error
> in printString: evaluate "self printString" to debug>, and in so doing
> reveals that the "self size" call fails in:
>> OrderedCollection>>do: elementBlock separatedBy: separatorBlock
>> 1 to: self size do:

IMHO this is not a problem. #printString doesn't have to work for
uninitialized objects.

Levente

> indicating a similar fix is potentially required for the following methods:
> OrderedCollection>>add: newObject afterIndex: index
> OrderedCollection>>add: newObject beforeIndex: index
> OrderedCollection>>at: index ifAbsentPut: block
> OrderedCollection>>collect: aBlock
> OrderedCollection>>copyReplaceFrom: start to: stop with:
> replacementCollection
> OrderedCollection>>makeRoomAtFirst
> OrderedCollection>>makeRoomAtLast
> OrderedCollection>>postCopyFrom: startIndex to: endIndex
> OrderedCollection>>with: otherCollection collect: twoArgBlock
> OrderedCollection>>withIndexCollect: elementAndIndexBlock
> which is not as clean as modifying OrderedCollection>>size, and still a bit
> uncertain.
>
> What is the community's thoughts on this?
>
> Finally, if any of these are desirable fixes, I would appreciate using my
> first contribution as a practice for submitting to the Inbox (if someone
> could direct me to some documentation that describes how to do this.)
>
> cheers, Ben
>
>
>
>
>
>
>

Eliot Miranda-2

Re: OrderedCollection>>size fails in debugger

On Sun, Jul 10, 2011 at 10:53 AM, Levente Uzonyi <[hidden email]> wrote:

On Mon, 11 Jul 2011, Ben Coman wrote:

I am writing some code that uses OrderedCollection and was tracing through it with the debugger when the debugger itself threw a DNU exception. I narrowed down reproduction of this to executing the following line, clicking <Debug> then eight times <Into> :

self halt. OrderedCollection new: 5.

As far as I can determine, the root cause is that "OrderedCollectionInspector>>fieldList" calls "object size" which has the following implementation:

OrderedCollection>>size
^ lastIndex - firstIndex + 1

where "lastIndex" is nil (and also "firstIndex" is nil).
To narrow it down further, the error can also be reproduced by executing the following:

OrderedCollectionInspector openOn: OrderedCollection basicNew.

I fixed this with:

OrderedCollection>>size
lastIndex ifNil: [ ^0 ].
^ lastIndex - firstIndex + 1

It would be more correct to also have included "firstIndex ifNil: [ ^0 ]" but I assume both conditions occur at the same time, so I left it out for performance. Not knowing how performance issue are balanced in Squeak, I was concerned that the "size" method might be called very often, and that even this one extra line might want to be avoided - I assume that this case only occurs in debugging at times when the object has not been fully initialized, and this error would not occur from application code. So I went looking for an

Right.

alternative. Back in the caller, rather than:

OrderedCollection>>fieldList
object ifNil: [ ^ OrderedCollection new].
^ self baseFieldList ,
(object size <= (self i1 + self i2)

<snip>

this corrected the problem:

OrderedCollection>>fieldList
object ifNil: [ ^ OrderedCollection new].
[ object size ] on: Exception do: [ ^ self baseFieldList ].
^ self baseFieldList ,
(object size <= (self i1 + self i2)

<snip>
I assume this would also be required for OrderedCollection>>selectedObjectIndex.

Yes, OrderedCollectionInspector should be changed, because that's what causes the problem. I'd simply create an #objectSize method for it and use that instead of object size.

However now in the debugger inst-var sub-pane, selecting self shows "<error in printString: evaluate "self printString" to debug>, and in so doing reveals that the "self size" call fails in:

OrderedCollection>>do: elementBlock separatedBy: separatorBlock
1 to: self size do:

IMHO this is not a problem. #printString doesn't have to work for uninitialized objects.

+100. One can't distort production code for the benefit of debugging. One can make the debugger robust, e.g. in the presence of faulty printing code, and the debugger and inspector already are robust in this way. I don't see the need of any changes here (other than the arguable firstIndex ifNil: [^0]".

Levente

indicating a similar fix is potentially required for the following methods:
OrderedCollection>>add: newObject afterIndex: index
OrderedCollection>>add: newObject beforeIndex: index
OrderedCollection>>at: index ifAbsentPut: block
OrderedCollection>>collect: aBlock
OrderedCollection>>copyReplaceFrom: start to: stop with: replacementCollection
OrderedCollection>>makeRoomAtFirst
OrderedCollection>>makeRoomAtLast
OrderedCollection>>postCopyFrom: startIndex to: endIndex
OrderedCollection>>with: otherCollection collect: twoArgBlock
OrderedCollection>>withIndexCollect: elementAndIndexBlock
which is not as clean as modifying OrderedCollection>>size, and still a bit uncertain.

What is the community's thoughts on this?

Finally, if any of these are desirable fixes, I would appreciate using my first contribution as a practice for submitting to the Inbox (if someone could direct me to some documentation that describes how to do this.)

cheers, Ben

--
best,

Eliot

Ben Coman

Re: OrderedCollection>>size fails in debugger

In reply to this post by Frank Shearar-3

Frank Shearar wrote:
> * Update your (clean/virgin) Squeak trunk image.
>
What is the best way to do that?

Ben Coman

Re: OrderedCollection>>size fails in debugger

In reply to this post by Levente Uzonyi-2

Levente Uzonyi wrote:

> On Mon, 11 Jul 2011, Ben Coman wrote:
>> I am writing some code that uses OrderedCollection and was tracing
>> through it with the debugger when the debugger itself threw a DNU
>> exception. I narrowed down reproduction of this to executing the
>> following line, clicking <Debug> then eight times <Into> :
>>> self halt. OrderedCollection new: 5.
>> As far as I can determine, the root cause is that
>> "OrderedCollectionInspector>>fieldList" calls "object size" which has
>> the following implementation:
>>> OrderedCollection>>size
>>> ^ lastIndex - firstIndex + 1
>> where "lastIndex" is nil (and also "firstIndex" is nil).
> Back in the caller, rather than:
>>> OrderedCollection>>fieldList
>>> object ifNil: [ ^ OrderedCollection new].
>>> ^ self baseFieldList ,
>>> (object size <= (self i1 + self i2) <snip>
>>
>>
>> this corrected the problem:
>>> OrderedCollection>>fieldList
>>> object ifNil: [ ^ OrderedCollection new].
>>> [ object size ] on: Exception do: [ ^ self baseFieldList ].
>>> ^ self baseFieldList ,
>>> (object size <= (self i1 + self i2) <snip>
> Yes, OrderedCollectionInspector should be changed, because that's what
> causes the problem. I'd simply create an #objectSize method for it and
> use that instead of object size.
> Levente
>> cheers, Ben

Do you not like leaving the line "[ object size ] on: Exception do: [ ^
self baseFieldList ]" where it is ?

Was your thought to create #objectSize in OrderedCollection or
OrderedCollectionInspector? At first I assumed you meant
OrderedCollection>>objectSize since OrderedCollectionInspector would
still not know whether lastIndex was nil, and would seem to require the
same exception handling. But perhaps you meant that
OrderCollectionInspector>>objectSize should just wrap the exception
handling, and so would be...

OrderCollectionInspector>>objectSize
[ object size ] on: Exception do: [ ^ 0 ].
^ object size.

OrderCollectionInspector>>fieldList
object ifNil: [ ^ OrderedCollection new].
^ self baseFieldList ,
(self objectSize <= (self i1 + self i2)
ifTrue: [(1 to: self objectSize)
collect: [:i | i printString]]
ifFalse: [(1 to: self i1) , (self objectSize - (self i2-1)
to: self objectSize)
collect: [:i | i printString]])

Although I wonder if a cleaner implementation that exception handling
could be...

OrderCollection>>hasSize
lastIndex ifNil: [ ^false ].
firstIndex ifNil: [ ^false ].
^true.

OrderCollectionInspector>>objectSize
object hasSize ifTrue: [^ object size ] ifFalse: [ ^0 ].

Frank Shearar-3

Re: OrderedCollection>>size fails in debugger

In reply to this post by Ben Coman

On 11 July 2011 03:46, Ben Coman <[hidden email]> wrote:
> Frank Shearar wrote:
>>
>> * Update your (clean/virgin) Squeak trunk image.
>>
>
> What is the best way to do that?

I always keep a clean one to hand, and clone it when I want to work on
a project. If you don't have a clean trunk image, you can download a
relatively recent image here - http://ftp.squeak.org/trunk/ - and
update it (click the mouse face on the extreme left of the top menu
bar, "Update Squeak").

If you already have your change in an image, file it out and load it
into the clean image. This allows you to check precisely what it is
you'll be committing and, just as importantly, will mean that your
tests run from clean state (like expecting certain methods/classes to
exist).

To file out a change, I usually go to Tools, Dual Change Sorter.
Usually, the changeset "Unnamed1" will be selected in the left pane. I
create a new changeset in the right pane (alt-n/cmd-n or through the
context menu), and copy the changes I want to that new changeset. File
it out, switch to the clean trunk image, and open a File List (again
under Tools). Navigate to the file you want, and file it in. (The
tediousness of this operation is why I usually write my fix from
within a clean trunk image to start with!)

frank

Ben Coman

Re: OrderedCollection>>size fails in debugger

Thanks Frank. That was the detail I needed.

One thing though, "clicking on the mouse face..." - wow, I've been playing with Squeak for months and never noticed it. My perusal of the menus always starts with the "first" menu item "Projects."
I don't know of any other system with a menu under an icon like that. Interesting that I must have been trained out of even noticing it. For me, that little mouse icon is cute and all, but fails the Principal Of Least Surprise.

cheers, Ben

cheers, Ben

Frank Shearar wrote:

On 11 July 2011 03:46, Ben Coman [hidden email] wrote:

Frank Shearar wrote:

* Update your (clean/virgin) Squeak trunk image.

What is the best way to do that?


I always keep a clean one to hand, and clone it when I want to work on
a project. If you don't have a clean trunk image, you can download a
relatively recent image here - http://ftp.squeak.org/trunk/ - and
update it (click the mouse face on the extreme left of the top menu
bar, "Update Squeak").

If you already have your change in an image, file it out and load it
into the clean image. This allows you to check precisely what it is
you'll be committing and, just as importantly, will mean that your
tests run from clean state (like expecting certain methods/classes to
exist).

To file out a change, I usually go to Tools, Dual Change Sorter.
Usually, the changeset "Unnamed1" will be selected in the left pane. I
create a new changeset in the right pane (alt-n/cmd-n or through the
context menu), and copy the changes I want to that new changeset. File
it out, switch to the clean trunk image, and open a File List (again
under Tools). Navigate to the file you want, and file it in. (The
tediousness of this operation is why I usually write my fix from
within a clean trunk image to start with!)

frank

Bert Freudenberg

Re: OrderedCollection>>size fails in debugger

On 11.07.2011, at 13:54, Ben Coman wrote:

> Thanks Frank. That was the detail I needed.
>
> One thing though, "clicking on the mouse face..." - wow, I've been playing with Squeak for months and never noticed it. My perusal of the menus always starts with the "first" menu item "Projects."
> I don't know of any other system with a menu under an icon like that. Interesting that I must have been trained out of even noticing it. For me, that little mouse icon is cute and all, but fails the Principal Of Least Surprise.

That top-left menu item is fairly standard in many operating systems, e.g. Mac OS and GNOME. Some others put a similar item in the bottom left corner, e.g. KDE.

That said, adding a label ("Squeak") would make it more obvious that there is a menu.

- Bert -

Levente Uzonyi-2

Re: OrderedCollection>>size fails in debugger

In reply to this post by Ben Coman

On Mon, 11 Jul 2011, Ben Coman wrote:

> Levente Uzonyi wrote:
>> On Mon, 11 Jul 2011, Ben Coman wrote:
>>> I am writing some code that uses OrderedCollection and was tracing through
>>> it with the debugger when the debugger itself threw a DNU exception. I
>>> narrowed down reproduction of this to executing the following line,
>>> clicking <Debug> then eight times <Into> :
>>>> self halt. OrderedCollection new: 5.
>>> As far as I can determine, the root cause is that
>>> "OrderedCollectionInspector>>fieldList" calls "object size" which has the
>>> following implementation:
>>>> OrderedCollection>>size
>>>> ^ lastIndex - firstIndex + 1
>>> where "lastIndex" is nil (and also "firstIndex" is nil).
>> Back in the caller, rather than:
>>>> OrderedCollection>>fieldList
>>>> object ifNil: [ ^ OrderedCollection new].
>>>> ^ self baseFieldList ,
>>>> (object size <= (self i1 + self i2) <snip>
>>>
>>>
>>> this corrected the problem:
>>>> OrderedCollection>>fieldList
>>>> object ifNil: [ ^ OrderedCollection new].
>>>> [ object size ] on: Exception do: [ ^ self baseFieldList ].
>>>> ^ self baseFieldList ,
>>>> (object size <= (self i1 + self i2) <snip>
>> Yes, OrderedCollectionInspector should be changed, because that's what
>> causes the problem. I'd simply create an #objectSize method for it and use
>> that instead of object size.
>> Levente
>>> cheers, Ben
>
> Do you not like leaving the line "[ object size ] on: Exception do: [ ^ self
> baseFieldList ]" where it is ?

I don't. :)

>
> Was your thought to create #objectSize in OrderedCollection or
> OrderedCollectionInspector? At first I assumed you meant

OrderedCollectionInspector.

> OrderedCollection>>objectSize since OrderedCollectionInspector would still
> not know whether lastIndex was nil, and would seem to require the same
> exception handling. But perhaps you meant that
> OrderCollectionInspector>>objectSize should just wrap the exception handling,
> and so would be...
>
> OrderCollectionInspector>>objectSize
> [ object size ] on: Exception do: [ ^ 0 ].
> ^ object size.

Kinda, but see what Nicolas wrote. Using his version (besides not
evaluating "object size" twice for initialized objects) uses the stack
optimally - no extra pushes, just use what's on the top. Also catching all
Exceptions is unwelcome, you should only catch errors.

>
> OrderCollectionInspector>>fieldList
> object ifNil: [ ^ OrderedCollection new].
> ^ self baseFieldList ,
> (self objectSize <= (self i1 + self i2)
> ifTrue: [(1 to: self objectSize)
> collect: [:i | i printString]]
> ifFalse: [(1 to: self i1) , (self objectSize - (self i2-1) to:
> self objectSize)
> collect: [:i | i printString]])
>
> Although I wonder if a cleaner implementation that exception handling could
> be...
>
> OrderCollection>>hasSize
> lastIndex ifNil: [ ^false ].
> firstIndex ifNil: [ ^false ].
> ^true.
>
> OrderCollectionInspector>>objectSize
> object hasSize ifTrue: [^ object size ] ifFalse: [ ^0 ].

I think the other solution is better, because it doesn't extend/modify
OrderedCollection.

Levente

>
>
>
>
>
>
>
>