Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2677133002:
[test] Cleanup CHECK_EQ order. (Closed)
Description[test] Cleanup CHECK_EQ order.
Keep the order in CHECK_EQ calls consistent as
(expected, actual).
Simplify CHECK_EQ(true, expected) to CHECK(expected) and
CHECK_EQ(false, expected) to CHECK(!expected).
BUG=
Review-Url: https://codereview.chromium.org/2677133002
Cr-Commit-Position: refs/heads/master@{#42964}
Committed: https://chromium.googlesource.com/v8/v8/+/a495fc92daf0adef64a95e858cd1fe07a430c2e2
Patch Set 1 #
Total comments: 2
Patch Set 2 : [test] Cleanup CHECK_EQ(true/false). #Patch Set 3 : Reparent to master. #
Dependent Patchsets: Messages
Total messages: 25 (18 generated)
The CQ bit was checked by franzih@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...
franzih@chromium.org changed reviewers: + yangguo@chromium.org
Hi Yang, I realized I used the wrong order in CHECK_EQ(). Cleaned up the whole file for consistency. PTAL. Thanks, Franzi
lgtm. but... https://codereview.chromium.org/2677133002/diff/1/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/2677133002/diff/1/test/cctest/test-api.cc#new... test/cctest/test-api.cc:511: + CHECK_EQ(false, source->IsExternal()); You could just do a CHECK(!source->IsExternal()) here. And everywhere below where the expectation is either true or false.
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 franzih@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...
Description was changed from ========== [test] Cleanup CHECK_EQ order. Keep the order in CHECK_EQ calls consistent as (expected, actual). BUG= ========== to ========== [test] Cleanup CHECK_EQ order. Keep the order in CHECK_EQ calls consistent as (expected, actual). Simplify CHECK_EQ(true, expected) to CHECK(expected) and CHECK_EQ(false, expected) to CHECK(!expected). BUG= ==========
https://codereview.chromium.org/2677133002/diff/1/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/2677133002/diff/1/test/cctest/test-api.cc#new... test/cctest/test-api.cc:511: + CHECK_EQ(false, source->IsExternal()); On 2017/02/06 10:43:43, Yang wrote: > You could just do a CHECK(!source->IsExternal()) here. And everywhere below > where the expectation is either true or false. Haha, true. The sed command grepping for true/false should have been a pretty clear hint. Simplified as well.
On 2017/02/06 11:45:13, Franzi wrote: > https://codereview.chromium.org/2677133002/diff/1/test/cctest/test-api.cc > File test/cctest/test-api.cc (right): > > https://codereview.chromium.org/2677133002/diff/1/test/cctest/test-api.cc#new... > test/cctest/test-api.cc:511: + CHECK_EQ(false, source->IsExternal()); > On 2017/02/06 10:43:43, Yang wrote: > > You could just do a CHECK(!source->IsExternal()) here. And everywhere below > > where the expectation is either true or false. > > Haha, true. The sed command grepping for true/false > should have been a pretty clear hint. Simplified as well. lgtm.
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 franzih@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2674103002 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by franzih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2677133002/#ps80001 (title: "Reparent to master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486383859929510,
"parent_rev": "35a82866d7db157013dd81c3b30134ae0148cc98", "commit_rev":
"a495fc92daf0adef64a95e858cd1fe07a430c2e2"}
Message was sent while issue was closed.
Description was changed from ========== [test] Cleanup CHECK_EQ order. Keep the order in CHECK_EQ calls consistent as (expected, actual). Simplify CHECK_EQ(true, expected) to CHECK(expected) and CHECK_EQ(false, expected) to CHECK(!expected). BUG= ========== to ========== [test] Cleanup CHECK_EQ order. Keep the order in CHECK_EQ calls consistent as (expected, actual). Simplify CHECK_EQ(true, expected) to CHECK(expected) and CHECK_EQ(false, expected) to CHECK(!expected). BUG= Review-Url: https://codereview.chromium.org/2677133002 Cr-Commit-Position: refs/heads/master@{#42964} Committed: https://chromium.googlesource.com/v8/v8/+/a495fc92daf0adef64a95e858cd1fe07a43... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/v8/v8/+/a495fc92daf0adef64a95e858cd1fe07a43... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
