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 |
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 |
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 - |
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 |
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 > > > > > > > |
On Sun, Jul 10, 2011 at 10:53 AM, Levente Uzonyi <[hidden email]> wrote: On Mon, 11 Jul 2011, Ben Coman wrote: +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]".
-- best, Eliot |
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? |
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 ]. |
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 |
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 |
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 - |
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 > > > > > > > > |
Free forum by Nabble | Edit this page |