Bug 145638 – [ES6] Arrow function. Write tests for the controlFlowProfiler

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[GSkachkov]

To be sure and prevent regressions, it's worth writing a few simple tests for the controlFlowProfiler.
Check path JavaScriptCore/tests/controlFlowProfiler/* there is a standardized way of doing this.

[GSkachkov]

Created attachment 254489 [details]
Patch

[Saam Barati]

This looks good to me. You should also have a test that
tests the assumption for range of the entire function itself.
The way this works for functions now is that the entire
function executing offset will begin from the "function"
key word and will end with the closing "}" of the function.
We do this by passing in different parts of the function that
represent the textual boundary of the function.

We should match this style with arrow function syntax so the beginning
should be the "(" in the parameter list, or the beginning letter
of the parameter in the single parameter case. For example, I'd make tests
like this:

```
function foo1(test) {
   var f1 = () => { "hello"; }
   if (test)
       f1();
}
foo1(false);
checkBasicBlock(foo1, '() =>', ShouldNotHaveExecuted);
checkBasicBlock(foo1, '; }', ShouldNotHaveExecuted);
foo1(true);
checkBasicBlock(foo1, '() =>', ShouldHaveExecuted);
checkBasicBlock(foo1, '; }', ShouldHaveExecuted);
// etc, maybe different offsets, too.

function foo2(test) {
   var f1 = (xyz) => { "hello" }
   if (test)
       f1();
}
foo1(false);
checkBasicBlock(foo1, '(xyz) =>', ShouldNotHaveExecuted);
checkBasicBlock(foo1, '; }', ShouldNotHaveExecuted);
foo1(true);
checkBasicBlock(foo1, '(xyz) =>', ShouldHaveExecuted);
checkBasicBlock(foo1, '; }', ShouldHaveExecuted);
// etc, maybe other tests with single parameter syntax, etc.

``` 

(Actually, looking through the tests for the control flow profiler, it doesn't look like there are other
tests that check function boundaries for regular functions. I should open a bug for that).

[Saam Barati]

nction foo2(test) {
>    var f1 = (xyz) => { "hello" }
>    if (test)
>        f1();
> }
> foo1(false);
> checkBasicBlock(foo1, '(xyz) =>', ShouldNotHaveExecuted);
> checkBasicBlock(foo1, '; }', ShouldNotHaveExecuted);
> foo1(true);
> checkBasicBlock(foo1, '(xyz) =>', ShouldHaveExecuted);
> checkBasicBlock(foo1, '; }', ShouldHaveExecuted);
Mistake: these should all be "foo2".

[Saam Barati]

Also, you can actually check the ranges out visually inside the Web Inspector
by enable the TypeProfiler/ControlFlowProfiler using the "T" button in the upper
right when looking at the source code.

[GSkachkov]

(In reply to comment #4)
> Also, you can actually check the ranges out visually inside the Web Inspector
> by enable the TypeProfiler/ControlFlowProfiler using the "T" button in the
> upper
> right when looking at the source code.

OK. I'll upload new tests today

[GSkachkov]

Created attachment 255624 [details]
Patch

[GSkachkov]

As for now need to wait until https://bugs.webkit.org/show_bug.cgi?id=144956 will be fixed. After that can be checked

[GSkachkov]

Created attachment 255700 [details]
Patch

[Saam Barati]

Comment on attachment 255700 [details]
Patch

Looks good to me.

[Joseph Pecoraro]

Is this stale?

[Saam Barati]

Can you make sure this still passes and then
I can r+/cq+. We may have changed some code that
affects this. Better to be safe.

[GSkachkov]

Created attachment 261578 [details]
Patch

Reupload

[GSkachkov]

Comment on attachment 261578 [details]
Patch

Looks like tests pass without errors

[Saam Barati]

Comment on attachment 261578 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 261578 [details]
Patch

Clearing flags on attachment: 261578

Committed r190042: <http://trac.webkit.org/changeset/190042>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.