The Inbox: ShoutCore-ct.73.mcz

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

The Inbox: ShoutCore-ct.73.mcz

commits-2
A new version of ShoutCore was added to project The Inbox:
http://source.squeak.org/inbox/ShoutCore-ct.73.mcz

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

Name: ShoutCore-ct.73
Author: ct
Time: 28 September 2019, 11:31:09.24777 pm
UUID: 27e05259-bafa-0749-827d-b54de84bb8d2
Ancestors: ShoutCore-ct.72

Fixes a bug in context-dependent styling, as described in [1].

Until proved otherwise, we cannot distinguish between args and tempvars via DebuggerMap, so shout-reparse the original context source up to the place where the provided context starts and access the active variables from the nested shout parser.

(Minimal example to reproduce the old bug: Do it:
[:x| [:z | (x*z) halt] value: 7] value: 6
And style [z] in the context of #halt - it was wrongly styled as a tempvar before.)

[1] http://forum.world.st/Distinguishing-temp-vars-and-arguments-via-DebuggerMap-td5104614.html

=============== Diff against ShoutCore-ct.72 ===============

Item was added:
+ ----- Method: SHParserST80>>activeArguments (in category 'accessing') -----
+ activeArguments
+ "Parsed arguments that are in the active scope"
+ ^ arguments!

Item was added:
+ ----- Method: SHParserST80>>activeTemporaries (in category 'accessing') -----
+ activeTemporaries
+ "Parsed temporaries that are in the active scope"
+ ^ temporaries!

Item was changed:
  ----- Method: SHParserST80>>initializeInstanceVariables (in category 'parse support') -----
  initializeInstanceVariables
 
  instanceVariables := classOrMetaClass
  ifNil: [ #() ]
  ifNotNil: [ classOrMetaClass allInstVarNames asArray ].
  allowUnderscoreAssignments := Scanner allowUnderscoreAsAssignment.
  allowUnderscoreSelectors := Scanner prefAllowUnderscoreSelectors.
  allowBlockArgumentAssignment := Scanner allowBlockArgumentAssignment.
  sourcePosition := 1.
  arguments
  ifNil: [ arguments := OrderedCollection with: nil ]
  ifNotNil: [ arguments reset; addLast: nil ].
  temporaries
  ifNil: [ temporaries := OrderedCollection with: nil ]
  ifNotNil: [ temporaries reset; addLast: nil ].
+ context ifNotNil: [ self initializeVariablesFromContext ].
- context ifNotNil: [
- | contextArgumentCount contextVariableNames |
- contextArgumentCount := context numArgs.
- contextVariableNames := context tempNames asOrderedCollection.
- contextArgumentCount > 0 ifTrue: [
- arguments at: 1 put: (contextVariableNames first: contextArgumentCount) ].
- contextArgumentCount < contextVariableNames size ifTrue: [
- temporaries at: 1 put: (contextVariableNames allButFirst: contextArgumentCount) ] ].
  bracketDepth := 0.
  ranges
  ifNil: [ ranges := OrderedCollection new: 40 "Covers over 80% of all methods." ]
  ifNotNil: [ ranges reset ]!

Item was added:
+ ----- Method: SHParserST80>>initializeVariablesFromContext (in category 'parse support') -----
+ initializeVariablesFromContext
+
+ | contextSourcePcIndex contextSourceParser |
+ contextSourcePcIndex := (context debuggerMap
+ rangeForPC: context pc
+ in: context method
+ contextIsActiveContext: true "little white lie to work in every situation")
+ start.
+ contextSourceParser := self class new
+ classOrMetaClass: context method methodClass;
+ environment: self environment;
+ source: (context method getSource first: contextSourcePcIndex);
+ yourself.
+ contextSourceParser parse.
+ arguments at: 1 put: ((contextSourceParser activeArguments
+ reject: #isNil) gather: #yourself) asOrderedCollection.
+ temporaries at: 1 put: ((contextSourceParser activeTemporaries
+ reject: #isNil) gather: #yourself) asOrderedCollection.!


Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: ShoutCore-ct.73.mcz

Levente Uzonyi
I don't think this is correct. It's clearly an improvement, but still
not perfect.
Even in your test case, you wrote #methodArg instead of #blockArg, and I
suppose those tests pass. But those variables are block arguments.
This is because you add all the contexts' variables to the top level,
which is supposed to hold the surrounding method's variables.
In my opinion, the context's block structure should be injected into
arguments and temporaries as they are written in that context.

Also, please use blocks instead of symbols for arguments of enumeration
methods (e.g. #reject:, #gather:). Using symbols is fine in scripts. They
might be okay in tests, but I don't think it's appropriate to use that
feature in Trunk code, no matter how comfortable it is. (I agree that the
code may seem more legible when you're used to it, but what about those
who are not?)

Levente

On Sat, 28 Sep 2019, [hidden email] wrote:

> A new version of ShoutCore was added to project The Inbox:
> http://source.squeak.org/inbox/ShoutCore-ct.73.mcz
>
> ==================== Summary ====================
>
> Name: ShoutCore-ct.73
> Author: ct
> Time: 28 September 2019, 11:31:09.24777 pm
> UUID: 27e05259-bafa-0749-827d-b54de84bb8d2
> Ancestors: ShoutCore-ct.72
>
> Fixes a bug in context-dependent styling, as described in [1].
>
> Until proved otherwise, we cannot distinguish between args and tempvars via DebuggerMap, so shout-reparse the original context source up to the place where the provided context starts and access the active variables from the nested shout parser.
>
> (Minimal example to reproduce the old bug: Do it:
> [:x| [:z | (x*z) halt] value: 7] value: 6
> And style [z] in the context of #halt - it was wrongly styled as a tempvar before.)
>
> [1] http://forum.world.st/Distinguishing-temp-vars-and-arguments-via-DebuggerMap-td5104614.html
>
> =============== Diff against ShoutCore-ct.72 ===============
>
> Item was added:
> + ----- Method: SHParserST80>>activeArguments (in category 'accessing') -----
> + activeArguments
> + "Parsed arguments that are in the active scope"
> + ^ arguments!
>
> Item was added:
> + ----- Method: SHParserST80>>activeTemporaries (in category 'accessing') -----
> + activeTemporaries
> + "Parsed temporaries that are in the active scope"
> + ^ temporaries!
>
> Item was changed:
>  ----- Method: SHParserST80>>initializeInstanceVariables (in category 'parse support') -----
>  initializeInstanceVariables
>
>   instanceVariables := classOrMetaClass
>   ifNil: [ #() ]
>   ifNotNil: [ classOrMetaClass allInstVarNames asArray ].
>   allowUnderscoreAssignments := Scanner allowUnderscoreAsAssignment.
>   allowUnderscoreSelectors := Scanner prefAllowUnderscoreSelectors.
>   allowBlockArgumentAssignment := Scanner allowBlockArgumentAssignment.
>   sourcePosition := 1.
>   arguments
>   ifNil: [ arguments := OrderedCollection with: nil ]
>   ifNotNil: [ arguments reset; addLast: nil ].
>   temporaries
>   ifNil: [ temporaries := OrderedCollection with: nil ]
>   ifNotNil: [ temporaries reset; addLast: nil ].
> + context ifNotNil: [ self initializeVariablesFromContext ].
> - context ifNotNil: [
> - | contextArgumentCount contextVariableNames |
> - contextArgumentCount := context numArgs.
> - contextVariableNames := context tempNames asOrderedCollection.
> - contextArgumentCount > 0 ifTrue: [
> - arguments at: 1 put: (contextVariableNames first: contextArgumentCount) ].
> - contextArgumentCount < contextVariableNames size ifTrue: [
> - temporaries at: 1 put: (contextVariableNames allButFirst: contextArgumentCount) ] ].
>   bracketDepth := 0.
>   ranges
>   ifNil: [ ranges := OrderedCollection new: 40 "Covers over 80% of all methods." ]
>   ifNotNil: [ ranges reset ]!
>
> Item was added:
> + ----- Method: SHParserST80>>initializeVariablesFromContext (in category 'parse support') -----
> + initializeVariablesFromContext
> +
> + | contextSourcePcIndex contextSourceParser |
> + contextSourcePcIndex := (context debuggerMap
> + rangeForPC: context pc
> + in: context method
> + contextIsActiveContext: true "little white lie to work in every situation")
> + start.
> + contextSourceParser := self class new
> + classOrMetaClass: context method methodClass;
> + environment: self environment;
> + source: (context method getSource first: contextSourcePcIndex);
> + yourself.
> + contextSourceParser parse.
> + arguments at: 1 put: ((contextSourceParser activeArguments
> + reject: #isNil) gather: #yourself) asOrderedCollection.
> + temporaries at: 1 put: ((contextSourceParser activeTemporaries
> + reject: #isNil) gather: #yourself) asOrderedCollection.!

Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: ShoutCore-ct.73.mcz

Christoph Thiede

Hello, thank you for the feedback!


The bug with #methodArg vs. #blockArg appears to be a bit older again. With injecting, do you mean they should be simply put at [arguments at: 2] and [temporaries at: 2]? Or are you talking about to nest the current source into a pseudo template that consists all declarations from context? In this case, I suppose we could not support #parseAMethod when context is not nil? Maybe you could clarify this :)


Regarding blocks: Hm, personally I find them intuitive to read, no matter whether the syntax is known (and the use of a hash is not that sophisticated). I think "select visible", "gather submorphs" etc. just have a clear natural language meaning. However, I did not want to start a debate of principles, I'm sure at least one like this has been conducted before my time ... Did the community ever resolve some coding standards?


Best,

Christoph


Von: Squeak-dev <[hidden email]> im Auftrag von Levente Uzonyi <[hidden email]>
Gesendet: Dienstag, 1. Oktober 2019 13:14:33
An: [hidden email]
Betreff: Re: [squeak-dev] The Inbox: ShoutCore-ct.73.mcz
 
I don't think this is correct. It's clearly an improvement, but still
not perfect.
Even in your test case, you wrote #methodArg instead of #blockArg, and I
suppose those tests pass. But those variables are block arguments.
This is because you add all the contexts' variables to the top level,
which is supposed to hold the surrounding method's variables.
In my opinion, the context's block structure should be injected into
arguments and temporaries as they are written in that context.

Also, please use blocks instead of symbols for arguments of enumeration
methods (e.g. #reject:, #gather:). Using symbols is fine in scripts. They
might be okay in tests, but I don't think it's appropriate to use that
feature in Trunk code, no matter how comfortable it is. (I agree that the
code may seem more legible when you're used to it, but what about those
who are not?)

Levente

On Sat, 28 Sep 2019, [hidden email] wrote:

> A new version of ShoutCore was added to project The Inbox:
> http://source.squeak.org/inbox/ShoutCore-ct.73.mcz
>
> ==================== Summary ====================
>
> Name: ShoutCore-ct.73
> Author: ct
> Time: 28 September 2019, 11:31:09.24777 pm
> UUID: 27e05259-bafa-0749-827d-b54de84bb8d2
> Ancestors: ShoutCore-ct.72
>
> Fixes a bug in context-dependent styling, as described in [1].
>
> Until proved otherwise, we cannot distinguish between args and tempvars via DebuggerMap, so shout-reparse the original context source up to the place where the provided context starts and access the active variables from the nested shout parser.
>
> (Minimal example to reproduce the old bug: Do it:
> [:x| [:z | (x*z) halt] value: 7] value: 6
> And style [z] in the context of #halt - it was wrongly styled as a tempvar before.)
>
> [1] http://forum.world.st/Distinguishing-temp-vars-and-arguments-via-DebuggerMap-td5104614.html
>
> =============== Diff against ShoutCore-ct.72 ===============
>
> Item was added:
> + ----- Method: SHParserST80>>activeArguments (in category 'accessing') -----
> + activeArguments
> +      "Parsed arguments that are in the active scope"
> +      ^ arguments!
>
> Item was added:
> + ----- Method: SHParserST80>>activeTemporaries (in category 'accessing') -----
> + activeTemporaries
> +      "Parsed temporaries that are in the active scope"
> +      ^ temporaries!
>
> Item was changed:
>  ----- Method: SHParserST80>>initializeInstanceVariables (in category 'parse support') -----
>  initializeInstanceVariables
>
>        instanceVariables := classOrMetaClass
>                ifNil: [ #() ]
>                ifNotNil: [ classOrMetaClass allInstVarNames asArray ].
>        allowUnderscoreAssignments := Scanner allowUnderscoreAsAssignment.
>        allowUnderscoreSelectors := Scanner prefAllowUnderscoreSelectors.
>        allowBlockArgumentAssignment := Scanner allowBlockArgumentAssignment.
>        sourcePosition := 1.
>        arguments
>                ifNil: [ arguments := OrderedCollection with: nil ]
>                ifNotNil: [ arguments reset; addLast: nil ].
>        temporaries
>                ifNil: [ temporaries := OrderedCollection with: nil ]
>                ifNotNil: [ temporaries reset; addLast: nil ].
> +      context ifNotNil: [ self initializeVariablesFromContext ].
> -      context ifNotNil: [
> -              | contextArgumentCount contextVariableNames |
> -              contextArgumentCount := context numArgs.
> -              contextVariableNames := context tempNames asOrderedCollection.
> -              contextArgumentCount > 0 ifTrue: [
> -                      arguments at: 1 put: (contextVariableNames first: contextArgumentCount) ].
> -              contextArgumentCount < contextVariableNames size ifTrue: [
> -                      temporaries at: 1 put: (contextVariableNames allButFirst: contextArgumentCount) ] ].
>        bracketDepth := 0.
>        ranges
>                ifNil: [ ranges := OrderedCollection new: 40 "Covers over 80% of all methods." ]
>                ifNotNil: [ ranges reset ]!
>
> Item was added:
> + ----- Method: SHParserST80>>initializeVariablesFromContext (in category 'parse support') -----
> + initializeVariablesFromContext
> +
> +      | contextSourcePcIndex contextSourceParser |
> +      contextSourcePcIndex := (context debuggerMap
> +              rangeForPC: context pc
> +              in: context method
> +              contextIsActiveContext: true "little white lie to work in every situation")
> +                      start.
> +      contextSourceParser := self class new
> +              classOrMetaClass: context method methodClass;
> +              environment: self environment;
> +              source: (context method getSource first: contextSourcePcIndex);
> +              yourself.
> +      contextSourceParser parse.
> +      arguments at: 1 put: ((contextSourceParser activeArguments
> +              reject: #isNil) gather: #yourself) asOrderedCollection.
> +      temporaries at: 1 put: ((contextSourceParser activeTemporaries
> +              reject: #isNil) gather: #yourself) asOrderedCollection.!



Carpe Squeak!
Reply | Threaded
Open this post in threaded view
|

Re: The Inbox: ShoutCore-ct.73.mcz

Levente Uzonyi
Hi Christoph,

On Tue, 1 Oct 2019, Thiede, Christoph wrote:

>
> Hello, thank you for the feedback!
>
>
> The bug with #methodArg vs. #blockArg appears to be a bit older again. With injecting, do you mean they should be simply put at [arguments at: 2] and [temporaries at: 2]? Or are you talking about to nest the current source
> into a pseudo template that consists all declarations from context? In this case, I suppose we could not support #parseAMethod when context is not nil? Maybe you could clarify this :)

The first slot of arguments and temporaries is for the method arguments
and temporaries, so, considering your test case, the outer block's
arguments and temporaries should be at index 2, and the inner blocks'
variables at index 3.
Changing the last lines of #initializeVariablesFromContext to

  arguments := contextSourceParser activeArguments.
  temporaries := contextSourceParser activeTemporaries

seems like a possible solution to me.

One more thing that came to my mind about using DebuggerMethodMap is
concurrent access. Since Shout can be run in the background, that should
be considered here too.

>
>
> Regarding blocks: Hm, personally I find them intuitive to read, no matter whether the syntax is known (and the use of a hash is not that sophisticated). I think "select visible", "gather submorphs" etc. just have a clear
> natural language meaning. However, I did not want to start a debate of principles, I'm sure at least one like this has been conducted before my time ... Did the community ever resolve some coding standards?

You're right. Quite a few such methods have slipped through over the
years.
We do not have any coding standards for the Trunk, and I doubt we could
agree on one.


Levente

>
>
> Best,
>
> Christoph
>
> _________________________________________________________________________________________________________________________________________________________________________________________________________________________________
> Von: Squeak-dev <[hidden email]> im Auftrag von Levente Uzonyi <[hidden email]>
> Gesendet: Dienstag, 1. Oktober 2019 13:14:33
> An: [hidden email]
> Betreff: Re: [squeak-dev] The Inbox: ShoutCore-ct.73.mcz  
> I don't think this is correct. It's clearly an improvement, but still
> not perfect.
> Even in your test case, you wrote #methodArg instead of #blockArg, and I
> suppose those tests pass. But those variables are block arguments.
> This is because you add all the contexts' variables to the top level,
> which is supposed to hold the surrounding method's variables.
> In my opinion, the context's block structure should be injected into
> arguments and temporaries as they are written in that context.
>
> Also, please use blocks instead of symbols for arguments of enumeration
> methods (e.g. #reject:, #gather:). Using symbols is fine in scripts. They
> might be okay in tests, but I don't think it's appropriate to use that
> feature in Trunk code, no matter how comfortable it is. (I agree that the
> code may seem more legible when you're used to it, but what about those
> who are not?)
>
> Levente
>
> On Sat, 28 Sep 2019, [hidden email] wrote:
>
> > A new version of ShoutCore was added to project The Inbox:
> > http://source.squeak.org/inbox/ShoutCore-ct.73.mcz
> >
> > ==================== Summary ====================
> >
> > Name: ShoutCore-ct.73
> > Author: ct
> > Time: 28 September 2019, 11:31:09.24777 pm
> > UUID: 27e05259-bafa-0749-827d-b54de84bb8d2
> > Ancestors: ShoutCore-ct.72
> >
> > Fixes a bug in context-dependent styling, as described in [1].
> >
> > Until proved otherwise, we cannot distinguish between args and tempvars via DebuggerMap, so shout-reparse the original context source up to the place where the provided context starts and access the active variables from
> the nested shout parser.
> >
> > (Minimal example to reproduce the old bug: Do it:
> > [:x| [:z | (x*z) halt] value: 7] value: 6
> > And style [z] in the context of #halt - it was wrongly styled as a tempvar before.)
> >
> > [1] http://forum.world.st/Distinguishing-temp-vars-and-arguments-via-DebuggerMap-td5104614.html
> >
> > =============== Diff against ShoutCore-ct.72 ===============
> >
> > Item was added:
> > + ----- Method: SHParserST80>>activeArguments (in category 'accessing') -----
> > + activeArguments
> > +      "Parsed arguments that are in the active scope"
> > +      ^ arguments!
> >
> > Item was added:
> > + ----- Method: SHParserST80>>activeTemporaries (in category 'accessing') -----
> > + activeTemporaries
> > +      "Parsed temporaries that are in the active scope"
> > +      ^ temporaries!
> >
> > Item was changed:
> >  ----- Method: SHParserST80>>initializeInstanceVariables (in category 'parse support') -----
> >  initializeInstanceVariables
> >
> >        instanceVariables := classOrMetaClass
> >                ifNil: [ #() ]
> >                ifNotNil: [ classOrMetaClass allInstVarNames asArray ].
> >        allowUnderscoreAssignments := Scanner allowUnderscoreAsAssignment.
> >        allowUnderscoreSelectors := Scanner prefAllowUnderscoreSelectors.
> >        allowBlockArgumentAssignment := Scanner allowBlockArgumentAssignment.
> >        sourcePosition := 1.
> >        arguments
> >                ifNil: [ arguments := OrderedCollection with: nil ]
> >                ifNotNil: [ arguments reset; addLast: nil ].
> >        temporaries
> >                ifNil: [ temporaries := OrderedCollection with: nil ]
> >                ifNotNil: [ temporaries reset; addLast: nil ].
> > +      context ifNotNil: [ self initializeVariablesFromContext ].
> > -      context ifNotNil: [
> > -              | contextArgumentCount contextVariableNames |
> > -              contextArgumentCount := context numArgs.
> > -              contextVariableNames := context tempNames asOrderedCollection.
> > -              contextArgumentCount > 0 ifTrue: [
> > -                      arguments at: 1 put: (contextVariableNames first: contextArgumentCount) ].
> > -              contextArgumentCount < contextVariableNames size ifTrue: [
> > -                      temporaries at: 1 put: (contextVariableNames allButFirst: contextArgumentCount) ] ].
> >        bracketDepth := 0.
> >        ranges
> >                ifNil: [ ranges := OrderedCollection new: 40 "Covers over 80% of all methods." ]
> >                ifNotNil: [ ranges reset ]!
> >
> > Item was added:
> > + ----- Method: SHParserST80>>initializeVariablesFromContext (in category 'parse support') -----
> > + initializeVariablesFromContext
> > +
> > +      | contextSourcePcIndex contextSourceParser |
> > +      contextSourcePcIndex := (context debuggerMap
> > +              rangeForPC: context pc
> > +              in: context method
> > +              contextIsActiveContext: true "little white lie to work in every situation")
> > +                      start.
> > +      contextSourceParser := self class new
> > +              classOrMetaClass: context method methodClass;
> > +              environment: self environment;
> > +              source: (context method getSource first: contextSourcePcIndex);
> > +              yourself.
> > +      contextSourceParser parse.
> > +      arguments at: 1 put: ((contextSourceParser activeArguments
> > +              reject: #isNil) gather: #yourself) asOrderedCollection.
> > +      temporaries at: 1 put: ((contextSourceParser activeTemporaries
> > +              reject: #isNil) gather: #yourself) asOrderedCollection.!
>
>
>