Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2182993002:
[LevelDBWrapper] Fix an issue that check condition is always true. (Closed)
|
Created:
4 years, 4 months ago by leonhsl(Using Gerrit) Modified:
4 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[LevelDBWrapper] Fix an issue that check condition is always true.
item->value.is_null() is always true, should check it.second.is_null() instead.
BUG=
Committed: https://crrev.com/e687684082856f27631233caf4a103b4709ef300
Cr-Commit-Position: refs/heads/master@{#408627}
Patch Set 1 #
Messages
Total messages: 19 (8 generated)
The CQ bit was checked by leon.han@intel.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...
leon.han@intel.com changed reviewers: + michaeln@chromium.org
Hi, would you PTAL to see whether this makes sense? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thnx lgtm (facepalm... no unittests)
leon.han@intel.com changed reviewers: + jochen@chromium.org
;-) +jochen Would you PTAL for OWNER review? Thanks.
can you add a test please?
On 2016/07/27 07:35:39, jochen wrote: > can you add a test please? This isn't used yet in production and the impl is incomplete. It's on my plate to complete it and I consider adding tests to be part of that project. So i'd say please feel free to commit this small bug fix w/o larger tests.
Hi, Jochen, considering that Michael will continue the implementations later and add tests from a wholesale point of view, may I just land this for now? Even we added a test here now, I'm afraid it maybe interfere with Michael's work later. Thanks~
not really thrilled by this, but since it's not yet used in production lgtm please make sure to add at least some unit test that constructs the class soon. that will make it easier to continue adding tests in the future
On 2016/07/29 09:37:09, jochen wrote: > not really thrilled by this, but since it's not yet used in production lgtm > > please make sure to add at least some unit test that constructs the class soon. > that will make it easier to continue adding tests in the future Got it and thanks~
The CQ bit was checked by leon.han@intel.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.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [LevelDBWrapper] Fix an issue that check condition is always true. item->value.is_null() is always true, should check it.second.is_null() instead. BUG= ========== to ========== [LevelDBWrapper] Fix an issue that check condition is always true. item->value.is_null() is always true, should check it.second.is_null() instead. BUG= Committed: https://crrev.com/e687684082856f27631233caf4a103b4709ef300 Cr-Commit-Position: refs/heads/master@{#408627} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/e687684082856f27631233caf4a103b4709ef300 Cr-Commit-Position: refs/heads/master@{#408627} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
