Mantis - Squeak
Viewing Issue Advanced Details
7572 Compiler minor always 11-15-10 08:22 11-15-11 03:38
leves  
leves  
normal  
resolved trunk  
fixed  
none    
none trunk  
0007572: Block-local unused temporaries are not handled properly
After accepting changes to a method in a browser, that has unused temporary variables, the parser used to ask if you want to remove them. This doesn't happen if the method has only block-local temporaries. The following example compiles without the question:

Object >> foo

    [ | bar | ] value


If the method also has unused method-local temporaries, then the Parser will ask if you want to remove the unused block-local temporaries, but they won't be removed if you answer yes. If ask the parser to remove the method-local temporaries, then it will ask the about the block-local temporaries twice. Example:

Object >> foo

    | foo |
    [ | bar | ] value

Another nice enhancement would be to remove the remaining || if all temoraries were removed. For example:

Object >> foo

    | foo |

will become

Object >> foo

    | |

after the parser removes the temporary.
 Parser-removeUnusedTemps-removedUnnecessaryNotification.st [^] (1,411 bytes) 05-19-11 17:38
 parser-lw.1.cs [^] (11,945 bytes) 05-27-11 18:23
 ParserErrorCorrection-lw.1.cs.gz [^] (1,129 bytes) 05-28-11 11:43
 ParserErrorCorrectionIncludingBothFixes-lw.2.cs.gz [^] (3,471 bytes) 05-31-11 15:46

Notes
(0013937)
chris   
11-16-10 22:24   
I prefer that we use syntax-highlighting to indicate an unused temporary. I find the computer prompting me for such a thing to be disruptive, especially when I'm working in a workspace and commenting out portions.

How ridiculous is it for the software to ask, "do you want me to delete it?" and, after selecting "yes", it says, "sorry I can't do that." It bugged me twice to tell me some manual intervention is required?! Awful.
(0013939)
leves   
11-17-10 03:40   
I think it's possible to modify Shout to highlight unused temporaries differently. The question is sometimes useful, so I wouldn't remove it. I like the feature, that the compiler can remove the unused temps. So if you want to get rid of the question, then add a preference for it (override the defaultAction of UnusedVariable).
(0014095)
Lars   
05-07-11 19:04   
For a seminar on development processes in open source projects, I choose this bug to solve it.
But because my account status is reporter, I can not assign me to this bug. Therefore this Note...

(0014096)
leves   
05-08-11 02:18   
Thanks Lars, it would be great to have this fixed. I guess only admins can assign this issue to you (I can only assign it to other "developers", to the "reporter" or to myself).
Note that the description of the issue got a bit messed up. It should be like: "If you ask the parser to remove the method local temporaries, then it will ask you about the block local temporaries twice."
(0014112)
Lars   
05-19-11 17:53   
I followed the debugger to the points of interest for these bug(s) and have some notes:
The attached file is a change-set to remove the annoying notifications, if an unused but assigned temporary is tried to be removed. In this version, the parser doesn't ask anymore, if there is an assignment to the temporary.

The parser asks twice for block local temporaries, because after the removal of temporaries (after source code changes actually), the changed source is parsed again. And because the block local temporaries are still unused, they are queried again.

Now to the problem which seemed main to me: Not removing the block local temporaries.
The block-temps are not removed, because the position of temporaries is saved to tempsMark during parsing only for the main method temp declaration.
I'll now make this tempsMark a list, to enable removal of block local temporaries as well. Afterwards, I'll look into the parser to see why removal is only queried if there are method local temporaries (case one in this issues description)
(0014131)
Lars   
05-27-11 18:34   
I found a solution.
The attached File parser-lw.1.cs contains the changes to the class Parser as well as a TestCase named BlockLocalTemporariesRemoval, because there has not yet been a Test for the Parser.

The changed Parser now can remove block-local and method-local temporaries, even if no method-local temporaries exist. It keeps the tempsMarker-Variables up to date and removes empty temp-definitions after removing unused variables.

The Test-Class has to implement the parsers requestor interface, therefore so many empty methods.

I added accessors to parser because I'm iterating over BlockNodes but somehow hte MethodNode generated by the Parser never had the tempsMark-variable set, therefore the Parser itself is added to the Collection I'm iterating over.

The problem about asking several times for temp removal originates from the fact that after changing the parsed method's source, it is reparsed and thus the user is asked again.

If there are any question, feel free to add notes here.

(0014132)
Lars   
05-28-11 11:45   
While preparing my talk, I noticed that I fixed the wrong defect. Therefore here another version with a fix to the n-times-asking problem.
Sorry for spamming all over this bug report.
(0014134)
leves   
06-01-11 21:09   
Great progress Lars! Is it possible to release these changes under the MIT license, so we can integrate them into the next Squeak release?
(0014135)
Lars   
06-01-11 21:11   
I'd be very happy. You may release it under the MIT or any other Open Source Definition compatible license.
(0014136)
leves   
06-01-11 21:31   
Thanks, I'll integrate them to the Trunk soon.
(0014193)
leves   
11-15-11 03:38   
Integrated the changeset: Compiler-ul.220 & Tests-ul.134.