Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2160563003:
Reinforce fast-accessor-assembler tests. (Closed)
DescriptionReinforce fast-accessor-assembler tests.
Add additional sanity checks for implicit parameters, mainly to prevent passing
a wrong context, which can lead to obscure errors.
This is a regression test after https://crrev.com/da4f2491501e54d436ed448c5e345480d1f5906b
BUG=chromium:508898
Committed: https://crrev.com/6a65cff40f3a079d9b0dc394e99d8972d1fd2f16
Cr-Commit-Position: refs/heads/master@{#37876}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add additional checks for holder #
Total comments: 1
Patch Set 3 : Avoid deprecated methods to peek properties #Patch Set 4 : Simplify dummy property setup #Messages
Total messages: 25 (16 generated)
Description was changed from ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lean to obscure errors. This is a regression test after 034c52a86d292dbb5992e7e16c82b56db0680fdc. BUG=chromium:508898 ========== to ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lean to obscure errors. This is a regression test after 034c52a86d292dbb5992e7e16c82b56db0680fdc. BUG=chromium:508898 ==========
peterssen@google.com changed reviewers: + vogelheim@chromium.org
Description was changed from ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lean to obscure errors. This is a regression test after 034c52a86d292dbb5992e7e16c82b56db0680fdc. BUG=chromium:508898 ========== to ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lean to obscure errors. This is a regression test after https://crrev.com/034c52a86d292dbb5992e7e16c82b56db0680fdc BUG=chromium:508898 ==========
Description was changed from ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lean to obscure errors. This is a regression test after https://crrev.com/034c52a86d292dbb5992e7e16c82b56db0680fdc BUG=chromium:508898 ========== to ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lean to obscure errors. This is a regression test after https://crrev.com/da4f2491501e54d436ed448c5e345480d1f5906b BUG=chromium:508898 ==========
Description was changed from ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lean to obscure errors. This is a regression test after https://crrev.com/da4f2491501e54d436ed448c5e345480d1f5906b BUG=chromium:508898 ========== to ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lead to obscure errors. This is a regression test after https://crrev.com/da4f2491501e54d436ed448c5e345480d1f5906b BUG=chromium:508898 ==========
PTAL
lgtm; but I'd be happier if we can also check the Holder somehow. https://codereview.chromium.org/2160563003/diff/1/test/cctest/test-api-fast-a... File test/cctest/test-api-fast-accessor-builder.cc (right): https://codereview.chromium.org/2160563003/diff/1/test/cctest/test-api-fast-a... test/cctest/test-api-fast-accessor-builder.cc:75: CHECK(info.Data()->IsUndefined()); Maybe we can check info.Holder()? This should be instantiated from the function template 'foo'. We could either check for presence of the test accessors, or alternatively 'mark' the foo instance w/ some property and check its presence here?
The CQ bit was checked by peterssen@google.com 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: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
lgtm, w/ comment. https://codereview.chromium.org/2160563003/diff/20001/test/cctest/test-api-fa... File test/cctest/test-api-fast-accessor-builder.cc (right): https://codereview.chromium.org/2160563003/diff/20001/test/cctest/test-api-fa... test/cctest/test-api-fast-accessor-builder.cc:378: } I think this is a bit more complex than necessary... since we're not planning to call the watermark, but merely check for its existence, something like this would also do: foo->CreateDataProperty(..., v8_str(kWaterMarkProperty), ..)
On 2016/07/19 14:44:58, vogelheim wrote: > lgtm, w/ comment. > > https://codereview.chromium.org/2160563003/diff/20001/test/cctest/test-api-fa... > File test/cctest/test-api-fast-accessor-builder.cc (right): > > https://codereview.chromium.org/2160563003/diff/20001/test/cctest/test-api-fa... > test/cctest/test-api-fast-accessor-builder.cc:378: } > I think this is a bit more complex than necessary... since we're not planning to > call the watermark, but merely check for its existence, something like this > would also do: > > foo->CreateDataProperty(..., v8_str(kWaterMarkProperty), ..) PTAL. Using .Set instead.
lgtm
The CQ bit was checked by peterssen@google.com 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.
The CQ bit was checked by peterssen@google.com
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 ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lead to obscure errors. This is a regression test after https://crrev.com/da4f2491501e54d436ed448c5e345480d1f5906b BUG=chromium:508898 ========== to ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lead to obscure errors. This is a regression test after https://crrev.com/da4f2491501e54d436ed448c5e345480d1f5906b BUG=chromium:508898 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lead to obscure errors. This is a regression test after https://crrev.com/da4f2491501e54d436ed448c5e345480d1f5906b BUG=chromium:508898 ========== to ========== Reinforce fast-accessor-assembler tests. Add additional sanity checks for implicit parameters, mainly to prevent passing a wrong context, which can lead to obscure errors. This is a regression test after https://crrev.com/da4f2491501e54d436ed448c5e345480d1f5906b BUG=chromium:508898 Committed: https://crrev.com/6a65cff40f3a079d9b0dc394e99d8972d1fd2f16 Cr-Commit-Position: refs/heads/master@{#37876} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6a65cff40f3a079d9b0dc394e99d8972d1fd2f16 Cr-Commit-Position: refs/heads/master@{#37876} |
