The Trunk: Collections-dtl.560.mcz

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

The Trunk: Collections-dtl.560.mcz

commits-2
David T. Lewis uploaded a new version of Collections to project The Trunk:
http://source.squeak.org/trunk/Collections-dtl.560.mcz

==================== Summary ====================

Name: Collections-dtl.560
Author: dtl
Time: 16 January 2014, 8:05:51.73 pm
UUID: 1ef138a7-e60c-4698-9cc4-c2f641e5607f
Ancestors: Collections-dtl.559

Relax parsing in RunArray>>scanFrom: to meet expectations of RunArrayTest>>testScanFromTrailer. The example string used in that test does not seem to match the implemention of the fileOut serialization, but the test may be right and the implementation wrong. In any case, relax the parsing here to meet the test expectations, and to be more consistent with the original version of the #scanFrom: method that assumed any unexpected charater to be a separator.

=============== Diff against Collections-dtl.559 ===============

Item was changed:
  ----- Method: RunArray class>>scanFrom: (in category 'instance creation') -----
  scanFrom: strm
  "Read the style section of a fileOut or sources file.  nextChunk has already been done.  We need to return a RunArray of TextAttributes of various kinds.  These are written by the implementors of writeScanOn:"
  | runs values attrList char |
  (strm peekFor: $( ) ifFalse: [^ nil].
  runs := OrderedCollection new.
  [strm skipSeparators.
  strm peekFor: $)] whileFalse:
  [runs add: (Number readFrom: strm)].
  values := OrderedCollection new. "Value array"
  attrList := OrderedCollection new. "Attributes list"
  [(char := strm peek) == nil] whileFalse: [
+ (char isSeparator or: [ char = $!! ])
+ ifTrue: [ "n.b. Skip $!! to meet expectations of RunArrayTest>>testScanFromTrailer.
+ The example string used in that test does not seem to match the implemention
+ of the fileOut serialization, but the test may be right and the implementation
+ wrong. In any case, relax the parsing here to meet the test expectations, and to
+ be more consistent with the original version of this method that assumed any
+ unexpected charater to be a separator. -dtl Jan 2014"
+ strm next "space, cr do nothing"]
- char isSeparator
- ifTrue: [strm next "space, cr do nothing"]
  ifFalse: [char == $,
  ifTrue: [strm next.
  values add: attrList asArray.
  attrList := OrderedCollection new]
  ifFalse: [attrList add:  (TextAttribute newFrom: strm)]
  ]
  ].
  values add: attrList asArray.
  ^ self runs: runs asArray values: (values copyFrom: 1 to: runs size) asArray
  "
  RunArray scanFrom: (ReadStream on: '(14 50 312)f1,f1b,f1LInteger +;i')
  "!


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Collections-dtl.560.mcz

Chris Muller-3
Thanks Dave, unfortunately I still cannot open this revised "Working
With Squeak".

File this method into trunk updated with this fix and then try to open
Help | Welcome Workspaces | Working with Squeak.

Same works after filing this method into Squeak4.5-13352 from the ftp
site or even 4.4.


On Thu, Jan 16, 2014 at 7:05 PM,  <[hidden email]> wrote:

> David T. Lewis uploaded a new version of Collections to project The Trunk:
> http://source.squeak.org/trunk/Collections-dtl.560.mcz
>
> ==================== Summary ====================
>
> Name: Collections-dtl.560
> Author: dtl
> Time: 16 January 2014, 8:05:51.73 pm
> UUID: 1ef138a7-e60c-4698-9cc4-c2f641e5607f
> Ancestors: Collections-dtl.559
>
> Relax parsing in RunArray>>scanFrom: to meet expectations of RunArrayTest>>testScanFromTrailer. The example string used in that test does not seem to match the implemention of the fileOut serialization, but the test may be right and the implementation wrong. In any case, relax the parsing here to meet the test expectations, and to be more consistent with the original version of the #scanFrom: method that assumed any unexpected charater to be a separator.
>
> =============== Diff against Collections-dtl.559 ===============
>
> Item was changed:
>   ----- Method: RunArray class>>scanFrom: (in category 'instance creation') -----
>   scanFrom: strm
>         "Read the style section of a fileOut or sources file.  nextChunk has already been done.  We need to return a RunArray of TextAttributes of various kinds.  These are written by the implementors of writeScanOn:"
>         | runs values attrList char |
>         (strm peekFor: $( ) ifFalse: [^ nil].
>         runs := OrderedCollection new.
>         [strm skipSeparators.
>          strm peekFor: $)] whileFalse:
>                 [runs add: (Number readFrom: strm)].
>         values := OrderedCollection new.        "Value array"
>         attrList := OrderedCollection new.      "Attributes list"
>         [(char := strm peek) == nil] whileFalse: [
> +               (char isSeparator or: [ char = $!! ])
> +                       ifTrue: [ "n.b. Skip $!! to meet expectations of RunArrayTest>>testScanFromTrailer.
> +                                       The example string used in that test does not seem to match the implemention
> +                                       of the fileOut serialization, but the test may be right and the implementation
> +                                       wrong. In any case, relax the parsing here to meet the test expectations, and to
> +                                       be more consistent with the original version of this method that assumed any
> +                                       unexpected charater to be a separator. -dtl Jan 2014"
> +                               strm next "space, cr do nothing"]
> -               char isSeparator
> -                       ifTrue: [strm next "space, cr do nothing"]
>                         ifFalse: [char == $,
>                                         ifTrue: [strm next.
>                                                 values add: attrList asArray.
>                                                 attrList := OrderedCollection new]
>                                         ifFalse: [attrList add:  (TextAttribute newFrom: strm)]
>                                 ]
>                 ].
>         values add: attrList asArray.
>         ^ self runs: runs asArray values: (values copyFrom: 1 to: runs size) asArray
>   "
>   RunArray scanFrom: (ReadStream on: '(14 50 312)f1,f1b,f1LInteger +;i')
>   "!
>
>



TheWorldMainDockingBar-workingWithSqueak.st.gz (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Collections-dtl.560.mcz

David T. Lewis
Urk.

When I load the updated #workingWithSqueak, then step through this in
a debugger:

  TheWorldMainDockingBar new workingWithSqueak

then I look at the string that is being converted to text with
readStream nextChunkText, it seems that the string contains a bunch
of trailing null characters at the end of the RunArray. This does not
seem right. That said, the original version of RunaArray>>scanFrom:
would have eaten all those nulls without complaining, which is presumably
why it runs in Squeak4.5-13352.

How was the source for this version of #workingWithSqueak created? I am
worried that I might have broken something in the writeScanOn: methods
for TextAttributes that might have led to generating corrupt source
code for the workingWithSqueak method ... I hope not :-(

Thanks,
Dave

On Thu, Jan 16, 2014 at 09:05:31PM -0600, Chris Muller wrote:

> Thanks Dave, unfortunately I still cannot open this revised "Working
> With Squeak".
>
> File this method into trunk updated with this fix and then try to open
> Help | Welcome Workspaces | Working with Squeak.
>
> Same works after filing this method into Squeak4.5-13352 from the ftp
> site or even 4.4.
>
>
> On Thu, Jan 16, 2014 at 7:05 PM,  <[hidden email]> wrote:
> > David T. Lewis uploaded a new version of Collections to project The Trunk:
> > http://source.squeak.org/trunk/Collections-dtl.560.mcz
> >
> > ==================== Summary ====================
> >
> > Name: Collections-dtl.560
> > Author: dtl
> > Time: 16 January 2014, 8:05:51.73 pm
> > UUID: 1ef138a7-e60c-4698-9cc4-c2f641e5607f
> > Ancestors: Collections-dtl.559
> >
> > Relax parsing in RunArray>>scanFrom: to meet expectations of RunArrayTest>>testScanFromTrailer. The example string used in that test does not seem to match the implemention of the fileOut serialization, but the test may be right and the implementation wrong. In any case, relax the parsing here to meet the test expectations, and to be more consistent with the original version of the #scanFrom: method that assumed any unexpected charater to be a separator.
> >
> > =============== Diff against Collections-dtl.559 ===============
> >
> > Item was changed:
> >   ----- Method: RunArray class>>scanFrom: (in category 'instance creation') -----
> >   scanFrom: strm
> >         "Read the style section of a fileOut or sources file.  nextChunk has already been done.  We need to return a RunArray of TextAttributes of various kinds.  These are written by the implementors of writeScanOn:"
> >         | runs values attrList char |
> >         (strm peekFor: $( ) ifFalse: [^ nil].
> >         runs := OrderedCollection new.
> >         [strm skipSeparators.
> >          strm peekFor: $)] whileFalse:
> >                 [runs add: (Number readFrom: strm)].
> >         values := OrderedCollection new.        "Value array"
> >         attrList := OrderedCollection new.      "Attributes list"
> >         [(char := strm peek) == nil] whileFalse: [
> > +               (char isSeparator or: [ char = $!! ])
> > +                       ifTrue: [ "n.b. Skip $!! to meet expectations of RunArrayTest>>testScanFromTrailer.
> > +                                       The example string used in that test does not seem to match the implemention
> > +                                       of the fileOut serialization, but the test may be right and the implementation
> > +                                       wrong. In any case, relax the parsing here to meet the test expectations, and to
> > +                                       be more consistent with the original version of this method that assumed any
> > +                                       unexpected charater to be a separator. -dtl Jan 2014"
> > +                               strm next "space, cr do nothing"]
> > -               char isSeparator
> > -                       ifTrue: [strm next "space, cr do nothing"]
> >                         ifFalse: [char == $,
> >                                         ifTrue: [strm next.
> >                                                 values add: attrList asArray.
> >                                                 attrList := OrderedCollection new]
> >                                         ifFalse: [attrList add:  (TextAttribute newFrom: strm)]
> >                                 ]
> >                 ].
> >         values add: attrList asArray.
> >         ^ self runs: runs asArray values: (values copyFrom: 1 to: runs size) asArray
> >   "
> >   RunArray scanFrom: (ReadStream on: '(14 50 312)f1,f1b,f1LInteger +;i')
> >   "!
> >
> >



Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Collections-dtl.560.mcz

David T. Lewis
On Thu, Jan 16, 2014 at 11:48:59PM -0500, David T. Lewis wrote:

>
> On Thu, Jan 16, 2014 at 09:05:31PM -0600, Chris Muller wrote:
> >
> > Thanks Dave, unfortunately I still cannot open this revised "Working
> > With Squeak".
> >
> > File this method into trunk updated with this fix and then try to open
> > Help | Welcome Workspaces | Working with Squeak.
> >
> > Same works after filing this method into Squeak4.5-13352 from the ftp
> > site or even 4.4.
>
> Urk.
>
> When I load the updated #workingWithSqueak, then step through this in
> a debugger:
>
>   TheWorldMainDockingBar new workingWithSqueak
>
> then I look at the string that is being converted to text with
> readStream nextChunkText, it seems that the string contains a bunch
> of trailing null characters at the end of the RunArray. This does not
> seem right. That said, the original version of RunaArray>>scanFrom:
> would have eaten all those nulls without complaining, which is presumably
> why it runs in Squeak4.5-13352.
>
> How was the source for this version of #workingWithSqueak created? I am
> worried that I might have broken something in the writeScanOn: methods
> for TextAttributes that might have led to generating corrupt source
> code for the workingWithSqueak method ... I hope not :-(
And when I remove the embedded null characters from the source code of the
workingWithSqueak method (attached), it works as expected. So the problem
may be in whatever generated the string that is used in the workingWithSqueak
method.

Dave




TheWorldMainDockingBar-workingWithSqueak.st.gz (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Collections-dtl.560.mcz

Chris Muller-3
In reply to this post by David T. Lewis
> How was the source for this version of #workingWithSqueak created? I am
> worried that I might have broken something in the writeScanOn: methods
> for TextAttributes that might have led to generating corrupt source
> code for the workingWithSqueak method ... I hope not :-(

It was created the normal way -- by simply opening up that workspace,
editing it, and then saving it (Command+s).

Doing that updates the method source.

See the acceptBlock in TheWorldMainDockingBar>>#showWelcomeText:label:in:.

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Collections-dtl.560.mcz

David T. Lewis
>> How was the source for this version of #workingWithSqueak created? I am
>> worried that I might have broken something in the writeScanOn: methods
>> for TextAttributes that might have led to generating corrupt source
>> code for the workingWithSqueak method ... I hope not :-(
>
> It was created the normal way -- by simply opening up that workspace,
> editing it, and then saving it (Command+s).
>
> Doing that updates the method source.
>
> See the acceptBlock in TheWorldMainDockingBar>>#showWelcomeText:label:in:.
>

That means that I must have introduced some new bug. I will look for it
and provide a fix this weekend.

Dave


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Collections-dtl.560.mcz

Levente Uzonyi-2
In reply to this post by commits-2
On Fri, 17 Jan 2014, [hidden email] wrote:

> David T. Lewis uploaded a new version of Collections to project The Trunk:
> http://source.squeak.org/trunk/Collections-dtl.560.mcz
>
> ==================== Summary ====================
>
> Name: Collections-dtl.560
> Author: dtl
> Time: 16 January 2014, 8:05:51.73 pm
> UUID: 1ef138a7-e60c-4698-9cc4-c2f641e5607f
> Ancestors: Collections-dtl.559
>
> Relax parsing in RunArray>>scanFrom: to meet expectations of RunArrayTest>>testScanFromTrailer. The example string used in that test does not seem to match the implemention of the fileOut serialization, but the test may be right and the implementation wrong. In any case, relax the parsing here to meet the test expectations, and to be more consistent with the original version of the #scanFrom: method that assumed any unexpected charater to be a separator.
>
> =============== Diff against Collections-dtl.559 ===============
>
> Item was changed:
>  ----- Method: RunArray class>>scanFrom: (in category 'instance creation') -----
>  scanFrom: strm
>   "Read the style section of a fileOut or sources file.  nextChunk has already been done.  We need to return a RunArray of TextAttributes of various kinds.  These are written by the implementors of writeScanOn:"
>   | runs values attrList char |
>   (strm peekFor: $( ) ifFalse: [^ nil].
>   runs := OrderedCollection new.
>   [strm skipSeparators.
>   strm peekFor: $)] whileFalse:
>   [runs add: (Number readFrom: strm)].
>   values := OrderedCollection new. "Value array"
>   attrList := OrderedCollection new. "Attributes list"
>   [(char := strm peek) == nil] whileFalse: [
> + (char isSeparator or: [ char = $!! ])

I think this is a bug in SqueakSource here. There's actually just one
exclamation mark in the code.


Levente

> + ifTrue: [ "n.b. Skip $!! to meet expectations of RunArrayTest>>testScanFromTrailer.
> + The example string used in that test does not seem to match the implemention
> + of the fileOut serialization, but the test may be right and the implementation
> + wrong. In any case, relax the parsing here to meet the test expectations, and to
> + be more consistent with the original version of this method that assumed any
> + unexpected charater to be a separator. -dtl Jan 2014"
> + strm next "space, cr do nothing"]
> - char isSeparator
> - ifTrue: [strm next "space, cr do nothing"]
>   ifFalse: [char == $,
>   ifTrue: [strm next.
>   values add: attrList asArray.
>   attrList := OrderedCollection new]
>   ifFalse: [attrList add:  (TextAttribute newFrom: strm)]
>   ]
>   ].
>   values add: attrList asArray.
>   ^ self runs: runs asArray values: (values copyFrom: 1 to: runs size) asArray
>  "
>  RunArray scanFrom: (ReadStream on: '(14 50 312)f1,f1b,f1LInteger +;i')
>  "!
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Collections-dtl.560.mcz

David T. Lewis
In reply to this post by David T. Lewis
On Fri, Jan 17, 2014 at 11:51:00AM -0500, David T. Lewis wrote:

> >> How was the source for this version of #workingWithSqueak created? I am
> >> worried that I might have broken something in the writeScanOn: methods
> >> for TextAttributes that might have led to generating corrupt source
> >> code for the workingWithSqueak method ... I hope not :-(
> >
> > It was created the normal way -- by simply opening up that workspace,
> > editing it, and then saving it (Command+s).
> >
> > Doing that updates the method source.
> >
> > See the acceptBlock in TheWorldMainDockingBar>>#showWelcomeText:label:in:.
> >
>
> That means that I must have introduced some new bug. I will look for it
> and provide a fix this weekend.

Oh my goodness. This appears to be a remarkable confluence of of bugs.

The welcome text right now just happens to be a Text with a RunArray that
serializes to a string of exactly 101 characters, including a trailing comma
that needs to be trimmed. The RunArray>>writeScanOn: method backs the stream
up one character to trim off that extra comma, leaving the stream at position
100. It just so happens that the stream was initialized to size 100, so when
String>>new:streamContents: checks to see if the stream position matches the
original size, it sees that the position is 100 so yes they match and it must
be OK to simply return the #originalContents of the stream instead of going
to all the extra work of figuring out its current #contents. Unfortunately,
the stream has actually been advanced to position 101 before being backed
up to position 100, so its collection has been resized to 200 elements. So
we grab the 200 elements of the stream collection, assuming that these are
still the original 100 elements of the original stream collection. That
gives us a string with an extra comma and 99 null characters at the end
where we did not expect anything.

If I load our current #workingWithSqueak into an older Squeak image, the
above problem is still triggered, and the source code for #workingWithSqueak
gets corrupted. But it gets corrupted differently than in the latest Squeak,
presumably due to the changes that I recently made to TextAttributes. And
in the older Squeak, this mess does not result in an error when opening the
workingWithSqueak window, probably related to the more permissive parsing
in the original RunArray class>>scanFrom: parsing.

This is making my head hurt. I am going to think about it tomorrow.

"I can't think about that right now. If I do, I'll go crazy. I'll think about that tomorrow."
- Margaret Mitchell, Gone with the Wind

Dave


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: Collections-dtl.560.mcz

David T. Lewis
On Sat, Jan 18, 2014 at 07:29:42PM -0500, David T. Lewis wrote:

> On Fri, Jan 17, 2014 at 11:51:00AM -0500, David T. Lewis wrote:
> > >> How was the source for this version of #workingWithSqueak created? I am
> > >> worried that I might have broken something in the writeScanOn: methods
> > >> for TextAttributes that might have led to generating corrupt source
> > >> code for the workingWithSqueak method ... I hope not :-(
> > >
> > > It was created the normal way -- by simply opening up that workspace,
> > > editing it, and then saving it (Command+s).
> > >
> > > Doing that updates the method source.
> > >
> > > See the acceptBlock in TheWorldMainDockingBar>>#showWelcomeText:label:in:.
> > >
> >
> > That means that I must have introduced some new bug. I will look for it
> > and provide a fix this weekend.
>
> Oh my goodness. This appears to be a remarkable confluence of of bugs.
>
> The welcome text right now just happens to be a Text with a RunArray that
> serializes to a string of exactly 101 characters, including a trailing comma
> that needs to be trimmed. The RunArray>>writeScanOn: method backs the stream
> up one character to trim off that extra comma, leaving the stream at position
> 100. It just so happens that the stream was initialized to size 100, so when
> String>>new:streamContents: checks to see if the stream position matches the
> original size, it sees that the position is 100 so yes they match and it must
> be OK to simply return the #originalContents of the stream instead of going
> to all the extra work of figuring out its current #contents. Unfortunately,
> the stream has actually been advanced to position 101 before being backed
> up to position 100, so its collection has been resized to 200 elements. So
> we grab the 200 elements of the stream collection, assuming that these are
> still the original 100 elements of the original stream collection. That
> gives us a string with an extra comma and 99 null characters at the end
> where we did not expect anything.
>
> If I load our current #workingWithSqueak into an older Squeak image, the
> above problem is still triggered, and the source code for #workingWithSqueak
> gets corrupted. But it gets corrupted differently than in the latest Squeak,
> presumably due to the changes that I recently made to TextAttributes. And
> in the older Squeak, this mess does not result in an error when opening the
> workingWithSqueak window, probably related to the more permissive parsing
> in the original RunArray class>>scanFrom: parsing.

I put CollectionsTests-dtl.212 in the inbox with a test to document the
issue in OrderedCollectionTest>>testStreamContentsPositioning. I am not
sure how best to handle this, but with respect to the welcome workspace
issue for workingWithSqueak the workaround is to simply add or remove any
text attribute from the workspace text, after which the bug is no longer
triggered.

Dave