Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1410713009:
Add negative test case for CompareAndSwap. (Closed)
DescriptionAdd negative test case for CompareAndSwap.
Make sure that CompareAndSwap isn't doing things when it shouldn't. This
also doubles as an example of how to detect "failure" if someone
is trying to figure out how to use CompareAndSwap.
BUG=
Committed: https://crrev.com/85c5a0a91d503d4cf4b5101ec0c83e9299f7074b
Cr-Commit-Position: refs/heads/master@{#357555}
Patch Set 1 #
Total comments: 3
Patch Set 2 : added comment explaining test #
Total comments: 2
Patch Set 3 : removed double space #Messages
Total messages: 18 (5 generated)
bcwhite@chromium.org changed reviewers: + danakj@chromium.org, glider@chromium.org
Description was changed from ========== Add negative test case for CompareAndSwap. Make sure that CAs isn't doing things when it shouldn't. This also doubles as an example of how to detect "failure" if someone is trying to figure out how to use CAS. BUG= ========== to ========== Add negative test case for CompareAndSwap. Make sure that CAS isn't doing things when it shouldn't. This also doubles as an example of how to detect "failure" if someone is trying to figure out how to use CAS. BUG= ==========
> Make sure that CAS isn't doing things when it shouldn't. This Can you write out CompareAndSwap fully in the commit log?
LGTM https://codereview.chromium.org/1410713009/diff/1/base/atomicops_unittest.cc File base/atomicops_unittest.cc (right): https://codereview.chromium.org/1410713009/diff/1/base/atomicops_unittest.cc#... base/atomicops_unittest.cc:92: AtomicType fail = base::subtle::NoBarrier_CompareAndSwap(&value, 0, 2); nit: i'd use 0,1 again cuz "2" is more magic/changed and it's not actually important. can you add a comment above this line saying what case this is testing/explaining it?
Description was changed from ========== Add negative test case for CompareAndSwap. Make sure that CAS isn't doing things when it shouldn't. This also doubles as an example of how to detect "failure" if someone is trying to figure out how to use CAS. BUG= ========== to ========== Add negative test case for CompareAndSwap. Make sure that CompareAndSwap isn't doing things when it shouldn't. This also doubles as an example of how to detect "failure" if someone is trying to figure out how to use CompareAndSwap. BUG= ==========
https://codereview.chromium.org/1410713009/diff/1/base/atomicops_unittest.cc File base/atomicops_unittest.cc (right): https://codereview.chromium.org/1410713009/diff/1/base/atomicops_unittest.cc#... base/atomicops_unittest.cc:92: AtomicType fail = base::subtle::NoBarrier_CompareAndSwap(&value, 0, 2); On 2015/11/02 21:18:36, danakj wrote: > nit: i'd use 0,1 again cuz "2" is more magic/changed and it's not actually > important. If it uses "0,1" then it wouldn't be able to verify that the value was *not* changed since it's already "1". > can you add a comment above this line saying what case this is > testing/explaining it? Done.
LGTM :) https://codereview.chromium.org/1410713009/diff/1/base/atomicops_unittest.cc File base/atomicops_unittest.cc (right): https://codereview.chromium.org/1410713009/diff/1/base/atomicops_unittest.cc#... base/atomicops_unittest.cc:92: AtomicType fail = base::subtle::NoBarrier_CompareAndSwap(&value, 0, 2); On 2015/11/02 23:02:30, bcwhite wrote: > On 2015/11/02 21:18:36, danakj wrote: > > nit: i'd use 0,1 again cuz "2" is more magic/changed and it's not actually > > important. > > If it uses "0,1" then it wouldn't be able to verify that the value was *not* > changed since it's already "1". > > > > can you add a comment above this line saying what case this is > > testing/explaining it? > > Done. True!
lgtm https://codereview.chromium.org/1410713009/diff/20001/base/atomicops_unittest.cc File base/atomicops_unittest.cc (right): https://codereview.chromium.org/1410713009/diff/20001/base/atomicops_unittest... base/atomicops_unittest.cc:93: // expected number. CAS will always return the actual value of the Please remove the double space.
lgtm lgtm https://codereview.chromium.org/1410713009/diff/20001/base/atomicops_unittest.cc File base/atomicops_unittest.cc (right): https://codereview.chromium.org/1410713009/diff/20001/base/atomicops_unittest... base/atomicops_unittest.cc:93: // expected number. CAS will always return the actual value of the Please remove the double space.
lgtm lgtm lgtm
On 2015/11/02 23:06:45, Alexander Potapenko wrote: > lgtm > > lgtm > > lgtm Hm, now I know what happens if I triple-click the "Quick LGTM" button.
https://codereview.chromium.org/1410713009/diff/20001/base/atomicops_unittest.cc File base/atomicops_unittest.cc (right): https://codereview.chromium.org/1410713009/diff/20001/base/atomicops_unittest... base/atomicops_unittest.cc:93: // expected number. CAS will always return the actual value of the On 2015/11/02 23:06:43, Alexander Potapenko wrote: > Please remove the double space. Done. (As I said in our other review... It's simply *not possible* for me to type single spaces after periods. :-D)
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, glider@chromium.org Link to the patchset: https://codereview.chromium.org/1410713009/#ps40001 (title: "removed double space")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1410713009/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1410713009/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/85c5a0a91d503d4cf4b5101ec0c83e9299f7074b Cr-Commit-Position: refs/heads/master@{#357555} |
