Fix handling of boxed loop variable in CPS IR

Fix handling of boxed loop variable in CPS IR

[Siggi Cherem (dart-lang), 2015-10-23 01:20:40]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2015-10-23 01:39:57]

sigmund@google.com changed reviewers:
+ asgerf@google.com, johnniwinther@google.com

[Siggi Cherem (dart-lang), 2015-10-23 01:39:58]

https://codereview.chromium.org/1412663006/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1412663006/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2538: if (location == null)
continue;
I'm not certain that this is the right fix. What I'm seeing is that
boxedLoopVariables are also in capturedVariables if they are updated outside of
the loop-update section. In that case, we don't include it in the
capturedVariables map.

The example from the test where this happens looks like this:
    main() {
      var f;
      for (int n = 0; n < 10; n++) {
        int k = n;
        f = () => 'k $k n $n';
        k++;
      }
      print(f());
    }

`n` is in boxedLoopedVariables, but only `k` is in capturedVariables.

Rather than skipping it here, should this be handled differently by the
ClosureScope in `closure.dart`?

[asgerf, 2015-10-23 09:37:30]

https://codereview.chromium.org/1412663006/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1412663006/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2538: if (location == null)
continue;
On 2015/10/23 01:39:58, Siggi Cherem (dart-lang) wrote:
> I'm not certain that this is the right fix. What I'm seeing is that
> boxedLoopVariables are also in capturedVariables if they are updated outside
of
> the loop-update section. In that case, we don't include it in the
> capturedVariables map.
> 
> The example from the test where this happens looks like this:
>     main() {
>       var f;
>       for (int n = 0; n < 10; n++) {
>         int k = n;
>         f = () => 'k $k n $n';
>         k++;
>       }
>       print(f());
>     }
> 
> `n` is in boxedLoopedVariables, but only `k` is in capturedVariables.
> 
> Rather than skipping it here, should this be handled differently by the
> ClosureScope in `closure.dart`?

'n' is captured but not boxed, so it's absurd that it should be in
boxedLoopVariables and not capturedVariables.

I've proposed a fix in closure.dart here:
https://codereview.chromium.org/1409933004/

[Siggi Cherem (dart-lang), 2015-10-23 15:15:38]

On 2015/10/23 09:37:30, asgerf wrote:
>
https://codereview.chromium.org/1412663006/diff/20001/pkg/compiler/lib/src/cp...
> File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):
> 
>
https://codereview.chromium.org/1412663006/diff/20001/pkg/compiler/lib/src/cp...
> pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2538: if (location == null)
> continue;
> On 2015/10/23 01:39:58, Siggi Cherem (dart-lang) wrote:
> > I'm not certain that this is the right fix. What I'm seeing is that
> > boxedLoopVariables are also in capturedVariables if they are updated outside
> of
> > the loop-update section. In that case, we don't include it in the
> > capturedVariables map.
> > 
> > The example from the test where this happens looks like this:
> >     main() {
> >       var f;
> >       for (int n = 0; n < 10; n++) {
> >         int k = n;
> >         f = () => 'k $k n $n';
> >         k++;
> >       }
> >       print(f());
> >     }
> > 
> > `n` is in boxedLoopedVariables, but only `k` is in capturedVariables.
> > 
> > Rather than skipping it here, should this be handled differently by the
> > ClosureScope in `closure.dart`?
> 
> 'n' is captured but not boxed, so it's absurd that it should be in
> boxedLoopVariables and not capturedVariables.
> 
> I've proposed a fix in closure.dart here:
> https://codereview.chromium.org/1409933004/

Much better! Thanks for taking a look, that makes much more sense.