Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(221)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2081733002: [keys] support shadowing keys in the KeyAccumulator (Closed)

Created:
4 years, 6 months ago by Camillo Bruni
Modified:
4 years, 5 months ago
Reviewers:
adamk
CC:
caitp (gmail), v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[keys] support shadowing keys in the KeyAccumulator This cl fixes the long-standing bug for for-in with shadowing properties. BUG=v8:705 Committed: https://crrev.com/6b63d524c2006776f3c99d4edc7b9fc5144808e7 Cr-Commit-Position: refs/heads/master@{#37333}

Patch Set 1 #

Patch Set 2 : adding more tests #

Patch Set 3 : adding cctests #

Patch Set 4 : fixing hash set #

Patch Set 5 : rebasing #

Patch Set 6 : fixing hash creation #

Patch Set 7 : hardening ObjectHashSet::Has #

Patch Set 8 : rebasing #

Total comments: 9

Patch Set 9 : addressing comments #

Patch Set 10 : rebasing #

Unified diffs Side-by-side diffs Delta from patch set Stats (+439 lines, -115 lines) Patch
M src/elements.cc View 1 1 chunk +8 lines, -4 lines 0 comments Download
M src/keys.h View 1 2 3 4 5 6 7 8 9 4 chunks +22 lines, -6 lines 0 comments Download
M src/keys.cc View 1 2 3 4 5 6 7 8 9 9 chunks +96 lines, -28 lines 0 comments Download
M src/messages.cc View 1 2 3 4 8 9 1 chunk +1 line, -1 line 0 comments Download
M src/objects.h View 1 2 3 4 5 6 7 8 9 4 chunks +25 lines, -3 lines 0 comments Download
M src/objects.cc View 1 2 3 4 5 6 7 8 9 5 chunks +72 lines, -23 lines 0 comments Download
M src/objects-inl.h View 1 2 3 4 5 6 7 8 9 4 chunks +23 lines, -1 line 0 comments Download
M test/cctest/test-dictionary.cc View 1 2 3 4 5 6 1 chunk +68 lines, -0 lines 0 comments Download
M test/mjsunit/for-in.js View 1 1 chunk +124 lines, -49 lines 0 comments Download

Messages

Total messages: 23 (11 generated)
Camillo Bruni
PTAL this should work, somehow :P
4 years, 6 months ago (2016-06-23 17:09:32 UTC) #2
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2081733002/40001
4 years, 6 months ago (2016-06-23 17:09:43 UTC) #4
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/builds/3695) v8_linux64_avx2_rel_ng_triggered on ...
4 years, 6 months ago (2016-06-23 17:27:18 UTC) #6
adamk
Mostly lgtm, just a few questions/comments below (feel free to land once addressed, I don't ...
4 years, 5 months ago (2016-06-27 19:19:33 UTC) #7
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2081733002/160001
4 years, 5 months ago (2016-06-28 12:40:58 UTC) #9
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/builds/3968) v8_linux64_rel_ng on ...
4 years, 5 months ago (2016-06-28 12:42:30 UTC) #11
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2081733002/200001
4 years, 5 months ago (2016-06-28 12:50:59 UTC) #14
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 5 months ago (2016-06-28 13:17:23 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2081733002/200001
4 years, 5 months ago (2016-06-28 13:29:39 UTC) #19
commit-bot: I haz the power
Committed patchset #10 (id:200001)
4 years, 5 months ago (2016-06-28 13:32:26 UTC) #20
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/6b63d524c2006776f3c99d4edc7b9fc5144808e7 Cr-Commit-Position: refs/heads/master@{#37333}
4 years, 5 months ago (2016-06-28 13:33:41 UTC) #22
Camillo Bruni
4 years, 5 months ago (2016-06-29 17:27:56 UTC) #23
Message was sent while issue was closed. 
https://codereview.chromium.org/2081733002/diff/140001/src/keys.cc
File src/keys.cc (right):

https://codereview.chromium.org/2081733002/diff/140001/src/keys.cc#newcode514
src/keys.cc:514: bool is_shadowing_key = false;
On 2016/06/27 at 19:19:33, adamk wrote:
> This declaration seems like it'd be clearer inside the loop. Is it out here
for some reason I'm missing?

pushing it into the loop

https://codereview.chromium.org/2081733002/diff/140001/src/keys.h
File src/keys.h (right):

https://codereview.chromium.org/2081733002/diff/140001/src/keys.h#newcode72
src/keys.h:72: void AddShadowKey(Object* key);
On 2016/06/27 at 19:19:33, adamk wrote:
> Please add comments for these methods, outside of this code review and the
attached bug I suspect these names are going to be mysterious to a future
reader.

added more descriptive comments.

https://codereview.chromium.org/2081733002/diff/140001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2081733002/diff/140001/src/objects-inl.h#newc...
src/objects-inl.h:3219: CAST_ACCESSOR(ObjectHashSet)
On 2016/06/27 at 19:19:33, adamk wrote:
> Nit: looks like this list is in alphabetical order, please sort.

done.

https://codereview.chromium.org/2081733002/diff/140001/src/objects-inl.h#newc...
src/objects-inl.h:5124: return true;
On 2016/06/27 at 19:19:33, adamk wrote:
> This whitespace change looks wrong..git cl format's fault?

indeed, changing back

Powered by Google App Engine
This is Rietveld 408576698