[Cuis-dev] Removing unused temps from inside blocks
Juan Vuletich
juan at jvuletich.org
Thu Dec 19 16:20:19 PST 2019
Thanks for all this, folks!
Cheers,
--
Juan Vuletich
www.cuis-smalltalk.org
https://github.com/Cuis-Smalltalk/Cuis-Smalltalk-Dev
https://github.com/jvuletich
https://www.linkedin.com/in/juan-vuletich-75611b3
@JuanVuletich
On 12/17/2019 11:27 AM, Hernan Wilkinson via Cuis-dev wrote:
> Hi Eric,
> I integrated your changes with a small modification for the
> TemporaryToInstanceVariable refactor to keep working.
> I also added tests for unused variables.
> Now removing unused variables works correctly on block and removes
> temps. definition when no more is left.
>
> Cheers!
> Hernan
>
> On Fri, Dec 13, 2019 at 5:08 PM Eric Brandwein
> <brandweineric at gmail.com <mailto:brandweineric at gmail.com>> wrote:
>
> Hernán, I checked out the methods in the category "Debugger
> Support", but I just found ways of obtaining which are the
> temporary variables and their indexes given a program counter and
> a context, and not of determining the positions of those
> variables. There is another way, though: we could traverse the
> ParseNode tree searching for the BlockNode whose code ranges
> (obtained with Encoder>>#rangeForNode:ifAbsent:) contain the
> ranges of the variable we want to rename and also declares it, and
> then traverse the subtree of that BlockNode searching for the
> usages of that temporary. I'll write a class that implements this
> and send it to this thread.
>
> Another thing I wanted to say is that if my last ChangeSet is
> included in the image, we should change the
> TemporaryToInstanceVariable refactor too, because it assumes that
> the body of the MethodNode hasn't got the temporaries set, and so
> when one wants to transform a temporary declared in the method
> temporaries it says that there is already another block declaring
> the same temporary. I'll write a fix for that too.
>
> Cheers,
> Eric
>
> El vie., 13 dic. 2019 a las 9:51, Hernan Wilkinson
> (<hernan.wilkinson at 10pines.com
> <mailto:hernan.wilkinson at 10pines.com>>) escribió:
>
> Oh, I answered the previous mail without seeing this one...
> I'll take a look at it, but also if you can take a look at
> what I mentioned in the previous email it would be great.
>
> Hernan.
>
> On Thu, Dec 12, 2019 at 11:07 PM Eric Brandwein
> <brandweineric at gmail.com <mailto:brandweineric at gmail.com>> wrote:
>
> Here's the ChangeSet that goes through every BlockNode
> checking if its temporaries are unused, which fixes the
> issue of multiple temporaries with the same name in one
> method. I had to edit the main parsing method so as to add
> the temporaries to the BlockNode that represents the body.
> Given that every other BlockNode has its temporaries, it
> seemed logical that the body BlockNode should have them too.
>
>
>
> El jue., 12 dic. 2019 a las 20:04, Eric Brandwein
> (<brandweineric at gmail.com
> <mailto:brandweineric at gmail.com>>) escribió:
>
> Great! It works almost perfectly. The only bug that
> still stands is when there's two blocks in the same
> method with the same variable declared. For example,
> when saving this method:
>
> test
> [ | var | ].
> [ | var | var := 'var'. ^var ].
>
> It doesn't prompt to remove the variable var. This is
> because the scopeTable of the Encoder class just saves
> the last VariableNode when there are many with the
> same name. The same bug affects the RenameTemporary
> refactoring. In fact, this example shows that
> referencing a variable just by its name inside a
> method is not enough; one needs the block where it's
> declared, too.
>
> Given this, maybe the keys of the Encoder's scopeTable
> should not be the names of the variables, but instead
> objects with the name of the variable and the
> information of the block where it is declared for each
> one. Does this sound good? A couple of methods should
> be changed for this to work; all the
> Encoder#rangesFor..., for instance.
>
> Another possible solution for this case in particular
> would be to look BlockNode by BlockNode for unused
> temp names, and querying the positions for the
> VariableNodes found instead of for the names of the
> temporaries. This doesn't solve the bug in
> RenameTemporary though.
>
> Let me know what you think,
> Eric
>
> El jue., 12 dic. 2019 a las 17:47, Hernan Wilkinson
> (<hernan.wilkinson at 10pines.com
> <mailto:hernan.wilkinson at 10pines.com>>) escribió:
>
> Did it!! and also it removes the | if no more temp
> var is defined :-)
>
> At github now.
>
> Cheers!
> Hernan
>
> On Thu, Dec 12, 2019 at 4:39 PM Hernan Wilkinson
> <hernan.wilkinson at 10pines.com
> <mailto:hernan.wilkinson at 10pines.com>> wrote:
>
> I think I know how to fix it... instead of
> removing each variable, we should collect the
> ranges to remove and after going through all
> the temps remove them. That way there is no
> need to re parse... I'll give it a try
>
> On Thu, Dec 12, 2019 at 4:34 PM Hernan
> Wilkinson <hernan.wilkinson at 10pines.com
> <mailto:hernan.wilkinson at 10pines.com>> wrote:
>
> Hi Eric,
> thank you for the fix! It is in github now.
>
> Re-parsing is not a problem because
> methods are usually small in Smalltalk and
> the parser is fast enough, but it
> introduces an issue: when you say you
> don't want to remove the variable and then
> you remove another one, it will ask you
> again for the first one.
> I integrated the change anyway because it
> is a rare condition and it is better to
> remove unused temps on blocks.
> If you can fix that new issue, the
> better, but I think it will not be so
> simple...
>
> BTW, I also fixed another problem, it
> there was no temps in the method but
> unused temps in a block, it did not work.
>
> Thanks!
> Hernan.
>
> On Thu, Dec 12, 2019 at 3:03 AM Eric
> Brandwein via Cuis-dev
> <cuis-dev at lists.cuis.st
> <mailto:cuis-dev at lists.cuis.st>> wrote:
>
> Sorry, I just realised this doesn't
> work if you're erasing many variables
> at a time. The obvious solution is to
> just reparse each time a temporary
> variable is removed, and that is what
> this new attached ChangeSet does.
>
> This is maybe slow, and I guess the
> alternative would be to go through all
> the declarations of all blocks inside
> the parsed method from the bottom to
> the top, so as to not change the
> positions of other possibly unused
> declarations when removing one of
> them. Let me know if this other
> solution seems better or if the one
> that reparses every time is fine.
>
> Cheers,
> Eric
>
> El jue., 12 dic. 2019 a las 2:23, Eric
> Brandwein (<brandweineric at gmail.com
> <mailto:brandweineric at gmail.com>>)
> escribió:
>
> Hi all,
>
> Currently, when you save a method
> with an unused temporary variable
> declared, it prompts you to decide
> if you want to remove it or not.
> If you click yes, it removes the
> declaration, but only if it's
> declared in the method scope, and
> not if it is declared inside a block.
>
> To reproduce, just create a method
> with an unused temporary variable
> declared inside a block, save it,
> and click 'Yes' on the popup that
> appears.
>
> This is because the Parser only
> looks for the variable in the
> temps declared in the method
> scope. Here's a ChangeSet that
> fixes it making the Parser look
> inside blocks too, taking
> advantage of the fact that the
> Encoder has already found the
> positions for each variable.
>
> Cheers,
> Eric
>
> --
> Cuis-dev mailing list
> Cuis-dev at lists.cuis.st
> <mailto:Cuis-dev at lists.cuis.st>
> https://lists.cuis.st/mailman/listinfo/cuis-dev
>
>
>
> --
> *Hernán Wilkinson
> Agile Software Development, Teaching &
> Coaching*
> *Phone: +54-011*-4893-2057
> *Twitter: @HernanWilkinson*
> *site: http://www.10Pines.com
> <http://www.10pines.com/>*
> Address: Alem 896, Floor 6, Buenos Aires,
> Argentina
>
>
>
> --
> *Hernán Wilkinson
> Agile Software Development, Teaching & Coaching*
> *Phone: +54-011*-4893-2057
> *Twitter: @HernanWilkinson*
> *site: http://www.10Pines.com
> <http://www.10pines.com/>*
> Address: Alem 896, Floor 6, Buenos Aires,
> Argentina
>
>
>
> --
> *Hernán Wilkinson
> Agile Software Development, Teaching & Coaching*
> *Phone: +54-011*-4893-2057
> *Twitter: @HernanWilkinson*
> *site: http://www.10Pines.com
> <http://www.10pines.com/>*
> Address: Alem 896, Floor 6, Buenos Aires, Argentina
>
>
>
> --
> *Hernán Wilkinson
> Agile Software Development, Teaching & Coaching*
> *Phone: +54-011*-4893-2057
> *Twitter: @HernanWilkinson*
> *site: http://www.10Pines.com <http://www.10pines.com/>*
> Address: Alem 896, Floor 6, Buenos Aires, Argentina
>
>
>
> --
> *Hernán Wilkinson
> Agile Software Development, Teaching & Coaching*
> *Phone: +54-011*-4893-2057
> *Twitter: @HernanWilkinson*
> *site: http://www.10Pines.com <http://www.10pines.com/>*
> Address: Alem 896, Floor 6, Buenos Aires, Argentina
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cuis.st/mailman/archives/cuis-dev/attachments/20191219/7c951282/attachment-0001.htm>
More information about the Cuis-dev
mailing list