Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0007572 [Squeak] Compiler minor always 11-15-10 08:22 11-15-11 03:38
Reporter leves View Status public  
Assigned To leves
Priority normal Resolution fixed  
Status resolved   Product Version trunk
Summary 0007572: Block-local unused temporaries are not handled properly
Description 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

Additional Information 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.
Attached Files  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

- Relationships

- Notes
(0013937 - 434 - 476 - 476 - 476 - 476 - 476)
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 - 328 - 328 - 328 - 328 - 328 - 328)
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 - 196 - 214 - 214 - 214 - 214 - 214)
Lars
05-07-11 19:04
edited on: 05-07-11 19:06

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 - 383 - 419 - 419 - 419 - 419 - 419)
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 - 1044 - 1086 - 1086 - 1086 - 1086 - 1086)
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 - 1019 - 1097 - 1097 - 1097 - 1097 - 1097)
Lars
05-27-11 18:34
edited on: 05-27-11 18:39

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 - 184 - 190 - 190 - 190 - 190 - 190)
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 - 138 - 138 - 138 - 138 - 138 - 138)
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 - 107 - 107 - 107 - 107 - 107 - 107)
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 - 46 - 46 - 46 - 46 - 46 - 46)
leves
06-01-11 21:31

Thanks, I'll integrate them to the Trunk soon.
 
(0014193 - 57 - 61 - 61 - 61 - 61 - 61)
leves
11-15-11 03:38

Integrated the changeset: Compiler-ul.220 & Tests-ul.134.
 

- Issue History
Date Modified Username Field Change
11-15-10 08:22 leves New Issue
11-16-10 22:24 chris Note Added: 0013937
11-17-10 03:40 leves Note Added: 0013939
05-07-11 19:04 Lars Note Added: 0014095
05-07-11 19:06 Lars Note Edited: 0014095
05-08-11 02:18 leves Note Added: 0014096
05-19-11 17:35 Lars Issue Monitored: Lars
05-19-11 17:38 Lars File Added: Parser-removeUnusedTemps-removedUnnecessaryNotification.st
05-19-11 17:53 Lars Note Added: 0014112
05-27-11 18:23 Lars File Added: parser-lw.1.cs
05-27-11 18:34 Lars Note Added: 0014131
05-27-11 18:39 Lars Note Edited: 0014131
05-28-11 11:43 Lars File Added: ParserErrorCorrection-lw.1.cs.gz
05-28-11 11:45 Lars Note Added: 0014132
05-31-11 15:46 Lars File Added: ParserErrorCorrectionIncludingBothFixes-lw.2.cs.gz
06-01-11 21:09 leves Note Added: 0014134
06-01-11 21:11 Lars Note Added: 0014135
06-01-11 21:31 leves Note Added: 0014136
06-01-11 21:31 leves Status new => testing
11-15-11 03:38 leves Status testing => resolved
11-15-11 03:38 leves Fixed in Version  => trunk
11-15-11 03:38 leves Resolution open => fixed
11-15-11 03:38 leves Assigned To  => leves
11-15-11 03:38 leves Note Added: 0014193


Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
94 total queries executed.
52 unique queries executed.
Powered by Mantis Bugtracker