Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488493003:
[inspector] fix positions for single expression arrow function (Closed)
Description[inspector] fix positions for single expression arrow function
Currently function like "() => 239" contains offset 3 as begin of function and 8 as end of function.
This CL changes this to 6 and 9 respectively.
BUG=chromium:566801
R=yangguo@chromium.org,dgozman@chromium.org
TBR=adamk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/dd4ce25cdea9185bfeb1918e62066167378f2bb9
Cr-Commit-Position: refs/heads/master@{#40864}
Patch Set 1 #
Total comments: 5
Patch Set 2 : addressed comments #
Messages
Total messages: 38 (22 generated)
Dmitry and Yang, please take a look. I fixed some issues related to positions inside of arrow functions. I'm not sure what is best practice here. Instead of logic that I've added, I can just added additional flag into FunctionLiteral and set it into true right after "// Single-expression body" comment parser-base.h. Some problems are left - I'll fix them in follow up, added a todo.
The CQ bit was checked by dgozman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Exciting stuff! https://codereview.chromium.org/2488493003/diff/1/src/full-codegen/full-codeg... File src/full-codegen/full-codegen.cc (right): https://codereview.chromium.org/2488493003/diff/1/src/full-codegen/full-codeg... src/full-codegen/full-codegen.cc:682: void FullCodeGenerator::SetReturnPosition(FunctionLiteral* fun) { Do we have similar problem in other code generation/interpretation systems?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2488493003/diff/1/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2488493003/diff/1/src/ast/ast.h#newcode2712 src/ast/ast.h:2712: return body_->length() == 1 && I think adding another flag to FunctionLiteral makes a lot more sense than try to reverse engineer here. https://codereview.chromium.org/2488493003/diff/1/src/full-codegen/full-codeg... File src/full-codegen/full-codegen.cc (right): https://codereview.chromium.org/2488493003/diff/1/src/full-codegen/full-codeg... src/full-codegen/full-codegen.cc:682: void FullCodeGenerator::SetReturnPosition(FunctionLiteral* fun) { On 2016/11/08 01:37:13, dgozman wrote: > Do we have similar problem in other code generation/interpretation systems? Yes, we need this logic in BytecodeArrayBuilder::BytecodeArrayBuilder as well. Maybe wrap this entire thing into ast.h/ast.cc?
All done, please take a look! https://codereview.chromium.org/2488493003/diff/1/src/ast/ast.h File src/ast/ast.h (right): https://codereview.chromium.org/2488493003/diff/1/src/ast/ast.h#newcode2712 src/ast/ast.h:2712: return body_->length() == 1 && On 2016/11/08 12:23:33, Yang wrote: > I think adding another flag to FunctionLiteral makes a lot more sense than try > to reverse engineer here. Done. https://codereview.chromium.org/2488493003/diff/1/src/full-codegen/full-codeg... File src/full-codegen/full-codegen.cc (right): https://codereview.chromium.org/2488493003/diff/1/src/full-codegen/full-codeg... src/full-codegen/full-codegen.cc:682: void FullCodeGenerator::SetReturnPosition(FunctionLiteral* fun) { On 2016/11/08 12:23:33, Yang wrote: > On 2016/11/08 01:37:13, dgozman wrote: > > Do we have similar problem in other code generation/interpretation systems? > > Yes, we need this logic in BytecodeArrayBuilder::BytecodeArrayBuilder as well. > Maybe wrap this entire thing into ast.h/ast.cc? Done.
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by kozyatinskiy@chromium.org
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== [inspector] fix positions for single expression arrow function Currently function like "() => 239" comtains offset 3 as begin of function and 8 as end of function. This CL changes this to 6 and 9 respectively. BUG=chromium:566801 R=yangguo@chromium.org,dgozman@chromium.org ========== to ========== [inspector] fix positions for single expression arrow function Currently function like "() => 239" comtains offset 3 as begin of function and 8 as end of function. This CL changes this to 6 and 9 respectively. BUG=chromium:566801 R=yangguo@chromium.org,dgozman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/08 16:47:19, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > Committers are members of the group "project-v8-committers". > Note that this has nothing to do with OWNERS files. Weird. I thought you guys were committers by now. I'll check.
On 2016/11/09 06:19:22, Yang wrote: > On 2016/11/08 16:47:19, commit-bot: I haz the power wrote: > > No L-G-T-M from a valid reviewer yet. > > CQ run can only be started by full committers or once the patch has > > received an L-G-T-M from a full committer. > > Even if an L-G-T-M may have been provided, it was from a non-committer, > > _not_ a full super star committer. > > Committers are members of the group "project-v8-committers". > > Note that this has nothing to do with OWNERS files. > > Weird. I thought you guys were committers by now. I'll check. Seems like you weren't added yet, by accident. Fixed now.
Description was changed from ========== [inspector] fix positions for single expression arrow function Currently function like "() => 239" comtains offset 3 as begin of function and 8 as end of function. This CL changes this to 6 and 9 respectively. BUG=chromium:566801 R=yangguo@chromium.org,dgozman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== [inspector] fix positions for single expression arrow function Currently function like "() => 239" contains offset 3 as begin of function and 8 as end of function. This CL changes this to 6 and 9 respectively. BUG=chromium:566801 R=yangguo@chromium.org,dgozman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
On 2016/11/09 06:57:10, Yang wrote: > On 2016/11/09 06:19:22, Yang wrote: > > On 2016/11/08 16:47:19, commit-bot: I haz the power wrote: > > > No L-G-T-M from a valid reviewer yet. > > > CQ run can only be started by full committers or once the patch has > > > received an L-G-T-M from a full committer. > > > Even if an L-G-T-M may have been provided, it was from a non-committer, > > > _not_ a full super star committer. > > > Committers are members of the group "project-v8-committers". > > > Note that this has nothing to do with OWNERS files. > > > > Weird. I thought you guys were committers by now. I'll check. > > Seems like you weren't added yet, by accident. Fixed now. Thank you! I think it's my fault, I accidently set commit check box instead of dry run and still need review for this :)
LGTM.
bmeurer@chromium.org changed reviewers: + bmeurer@chromium.org
LGTM (rubber-stamped)
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/28153)
Description was changed from ========== [inspector] fix positions for single expression arrow function Currently function like "() => 239" contains offset 3 as begin of function and 8 as end of function. This CL changes this to 6 and 9 respectively. BUG=chromium:566801 R=yangguo@chromium.org,dgozman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== [inspector] fix positions for single expression arrow function Currently function like "() => 239" contains offset 3 as begin of function and 8 as end of function. This CL changes this to 6 and 9 respectively. BUG=chromium:566801 R=yangguo@chromium.org,dgozman@chromium.org TBR=adamk@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [inspector] fix positions for single expression arrow function Currently function like "() => 239" contains offset 3 as begin of function and 8 as end of function. This CL changes this to 6 and 9 respectively. BUG=chromium:566801 R=yangguo@chromium.org,dgozman@chromium.org TBR=adamk@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== [inspector] fix positions for single expression arrow function Currently function like "() => 239" contains offset 3 as begin of function and 8 as end of function. This CL changes this to 6 and 9 respectively. BUG=chromium:566801 R=yangguo@chromium.org,dgozman@chromium.org TBR=adamk@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] fix positions for single expression arrow function Currently function like "() => 239" contains offset 3 as begin of function and 8 as end of function. This CL changes this to 6 and 9 respectively. BUG=chromium:566801 R=yangguo@chromium.org,dgozman@chromium.org TBR=adamk@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== [inspector] fix positions for single expression arrow function Currently function like "() => 239" contains offset 3 as begin of function and 8 as end of function. This CL changes this to 6 and 9 respectively. BUG=chromium:566801 R=yangguo@chromium.org,dgozman@chromium.org TBR=adamk@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/dd4ce25cdea9185bfeb1918e62066167378f2bb9 Cr-Commit-Position: refs/heads/master@{#40864} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/dd4ce25cdea9185bfeb1918e62066167378f2bb9 Cr-Commit-Position: refs/heads/master@{#40864} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
