Smalltalk › Squeak › Squeak - Dev

Parser-removeUnusedTemps enhancement

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

6 messages Options

Romain Robbes-2

Parser-removeUnusedTemps enhancement

When accepting a method, if the parser encounters an unused temporary variable, it offers to remove it.

However half of the time the variable is stored into, and the parser does not remove it, instead telling that you should first remove the statement where the variable is stored.

With this fix (basically reordering the test and the user confirmation) only the variables which are truly unused are removed.

The code of the method is simpler too.

Romain

Romain Robbes

http://romain.robb.es

Parser-removeUnusedTemps.st (2K) Download Attachment

Damien Cassou-3

Re: Parser-removeUnusedTemps enhancement

Please open a bug report on bugs.squeak.org, write some unit tests and
add a link to your report in http://wiki.squeak.org/squeak/5934

2007/10/3, Romain Robbes <[hidden email]>:

>
>
> When accepting a method, if the parser encounters an unused temporary
> variable, it offers to remove it.
> However half of the time the variable is stored into, and the parser does
> not remove it, instead telling that you should first remove the statement
> where the variable is stored.
>
> With this fix (basically reordering the test and the user confirmation) only
> the variables which are truly unused are removed.
> The code of the method is simpler too.
>
> Romain
>
> --
> Romain Robbes
> http://romain.robb.es
>
>
>
>
>
>
>

--
Damien Cassou

Romain Robbes-2

Re: Parser-removeUnusedTemps enhancement

Well, writing a unit test for this will be a pain. It's a one-method change, should I really write a new test class compiling several methods, somehow bypassing user input to return an accurate answer, decompiling and comparing the source code with an expected result, and remove the temporary methods just for this?

I did add the bug report though (http://bugs.squeak.org/bug_view_advanced_page.php?bug_id=6704),

but editing of the wiki page requires a password.

Romain

On Oct 3, 2007, at 10:12 AM, Damien Cassou wrote:

Please open a bug report on bugs.squeak.org, write some unit tests and
add a link to your report in http://wiki.squeak.org/squeak/5934

2007/10/3, Romain Robbes <[hidden email]>:

When accepting a method, if the parser encounters an unused temporary
variable, it offers to remove it.
However half of the time the variable is stored into, and the parser does
not remove it, instead telling that you should first remove the statement
where the variable is stored.

With this fix (basically reordering the test and the user confirmation) only
the variables which are truly unused are removed.
The code of the method is simpler too.

Romain

--
Romain Robbes
http://romain.robb.es

--
Damien Cassou

Romain Robbes

http://romain.robb.es

Damien Cassou-3

Re: Parser-removeUnusedTemps enhancement

2007/10/3, Romain Robbes <[hidden email]>:
> Well, writing a unit test for this will be a pain. It's a one-method change,
> should I really write a new test class compiling several methods, somehow
> bypassing user input to return an accurate answer, decompiling and comparing
> the source code with an expected result, and remove the temporary methods
> just for this?

I'm just telling you what the release team is waiting for. But your
arguments are right :-).

> I did add the bug report though
> (http://bugs.squeak.org/bug_view_advanced_page.php?bug_id=6704),
> but editing of the wiki page requires a password.

login: squeak
pass: viewpoints

--
Damien Cassou

Bert Freudenberg

Re: Parser-removeUnusedTemps enhancement

On Oct 3, 2007, at 11:08 , Damien Cassou wrote:

> 2007/10/3, Romain Robbes <[hidden email]>:
>> Well, writing a unit test for this will be a pain. It's a one-
>> method change,
>> should I really write a new test class compiling several methods,
>> somehow
>> bypassing user input to return an accurate answer, decompiling and
>> comparing
>> the source code with an expected result, and remove the temporary
>> methods
>> just for this?
>
> I'm just telling you what the release team is waiting for. But your
> arguments are right :-).

I thought the requirement was to not break any unit tests. Requiring
every enhancement to come with a unit test is silly.

- Bert -

Ralph Johnson

Re: Parser-removeUnusedTemps enhancement

On 10/3/07, Bert Freudenberg <[hidden email]> wrote:
> I thought the requirement was to not break any unit tests. Requiring
> every enhancement to come with a unit test is silly.

Yes, that is the requirements. New unit tests are nice, but not
required. Code is more likely to be accepted faster if it comes with
new unit tests, but we'll take it case by case.

-Ralph