Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2765013003:
Add iterator for DocumentMarker::MarkerTypes (Closed)
DescriptionAdd iterator for DocumentMarker::MarkerTypes
This idea came up in code review for a DocumentMarkerController refactor:
https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode214
Review-Url: https://codereview.chromium.org/2765013003
Cr-Commit-Position: refs/heads/master@{#459159}
Committed: https://chromium.googlesource.com/chromium/src/+/9b3e9af19463c051da0da835f4181bba204169a3
Patch Set 1 #
Total comments: 9
Patch Set 2 : Make requested changes #
Total comments: 5
Patch Set 3 : Respond to comments #
Total comments: 2
Patch Set 4 : rightmost => least significant #
Total comments: 1
Patch Set 5 : Work around MSVC warning #
Dependent Patchsets: Messages
Total messages: 36 (17 generated)
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
rlanday@chromium.org changed required reviewers: + yosin@chromium.org
Thanks for the quick patch! https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:60: explicit MarkerTypesIterator(int markerTypes) s/int/unsigned/ https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:72: m_remainingTypes &= (m_remainingTypes - 1); add: DCHECK(m_remainingTypes); https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:77: return static_cast<MarkerType>(m_remainingTypes & -m_remainingTypes); add: DCHECK(m_remainingTypes); https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:81: int m_remainingTypes; s/int/unsigned/ https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp (right): https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:18: for (DocumentMarker::MarkerType type : markerTypes) { nit: unnecessary braces. https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:25: ASSERT_EQ(markerTypes.contains(type), typesFromIterator.contains(type)); I'm confused... yosin@: Is there any preference between EXPECT_FOO and ASSERT_FOO in unit tests?
https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp (right): https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:25: ASSERT_EQ(markerTypes.contains(type), typesFromIterator.contains(type)); On 2017/03/22 at 01:02:58, Xiaocheng wrote: > I'm confused... > > yosin@: Is there any preference between EXPECT_FOO and ASSERT_FOO in unit tests? This checks that for each possible combination of MarkerTypes, the set of types the iterator goes through is correct (by using MarkerTypes.contains() which we assume already works). I used ASSERT_EQ since if this test fails and we didn't immediately stop, there would be a ton of not-very-helpful output since this is inside two nested loops.
https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp (right): https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:22: for (int i = 0; i < DocumentMarker::MarkerTypeIndexesCount; ++i) { s/int/unsigned/, and use another variable name. https://codereview.chromium.org/2765013003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:25: ASSERT_EQ(markerTypes.contains(type), typesFromIterator.contains(type)); On 2017/03/22 at 01:38:25, rlanday wrote: > On 2017/03/22 at 01:02:58, Xiaocheng wrote: > > I'm confused... > > > > yosin@: Is there any preference between EXPECT_FOO and ASSERT_FOO in unit tests? > > This checks that for each possible combination of MarkerTypes, the set of types the iterator goes through is correct (by using MarkerTypes.contains() which we assume already works). > > I used ASSERT_EQ since if this test fails and we didn't immediately stop, there would be a ton of not-very-helpful output since this is inside two nested loops. Makes sense to use ASSERT_EQ here. To make the test useful, however, the fail message should also contain the value of |i| and |i| (just noticed that you used the same loop variable name...).
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2765013003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2765013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:58: class MarkerTypesIterator { Let's add std::iterator<std::forward_iterator_tag, MarkerType> See http://en.cppreference.com/w/cpp/iterator/iterator for more details. https://codereview.chromium.org/2765013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:68: return !(*this == other); How about |!operator==(other)| to be more explicit about implementation. https://codereview.chromium.org/2765013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:73: m_remainingTypes &= (m_remainingTypes - 1); Let's add a comment: turn off right most 1-bit (from Hacker's Delight) or better Manual calculation: 7 | 7 & 6 | 6 6 | 6 & 5 | 4 4 | 4 & 3 | 0 https://codereview.chromium.org/2765013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:79: return static_cast<MarkerType>(m_remainingTypes & -m_remainingTypes); Let's add comment: Isolate the right most 1-bit (from Hacker's Delight) Note: Manual calculation: 7 | 7 & -7 | 1 6 | 6 & -6 | 2 4 | 4 & -4 | 4 https://codereview.chromium.org/2765013003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp (right): https://codereview.chromium.org/2765013003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerTest.cpp:13: TEST_F(DocumentMarkerTest, MarkerTypesIterator) { I would like to have explicit test cases rather than test cases generated by program. e.g. TEST_F(DocumentMarkerTest, MarkerTypeIteratorEmpty) { MarkerTypes types(0); EXPECT_TRUE(types.begin() == types.end()); } TEST_F(DocumentMarkerTest, MarkerTypeIteratorOne) { MarkerTypes types(DocumentMarker::Spelling); ASSERT_TRUE(types.begin() != types.end()); auto& it = type.begin(); EXPECT_EQ(DocumentMarker::Spelling, *it); ++it; EXPECT_TRUE(it == types.end()); } TEST_F(DocumentMarkerTest, MarkerTypeIteratorConsecutive) { MarkerTypes types(0b11); ... } TEST_F(DocumentMarkerTest, MarkerTypeIteratorDistributed) { MarkerTypes types(0b101); ... }
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2765013003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2765013003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:74: // Turn off rightmost 1-bit (from Hacker's Delight 2-1) nit: I prefer "least significant bit" to be more precise https://codereview.chromium.org/2765013003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:85: // Isolate rightmost 1-bit (from Hacker's Delight 2-1) nit: ditto
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/03/22 at 17:57:05, xiaochengh wrote: > https://codereview.chromium.org/2765013003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): > > https://codereview.chromium.org/2765013003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:74: // Turn off rightmost 1-bit (from Hacker's Delight 2-1) > nit: I prefer "least significant bit" to be more precise > > https://codereview.chromium.org/2765013003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:85: // Isolate rightmost 1-bit (from Hacker's Delight 2-1) > nit: ditto Updated
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2765013003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): https://codereview.chromium.org/2765013003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:90: return static_cast<MarkerType>(m_remainingTypes & -m_remainingTypes); xiaochengh, MSVC is throwing a compiler warning on this line: warning C4146: unary minus operator applied to unsigned type, result still unsigned I think m_remainingTypes should be signed but you said to make it unsigned, can I change it back?
On 2017/03/22 at 18:38:18, rlanday wrote: > https://codereview.chromium.org/2765013003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): > > https://codereview.chromium.org/2765013003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:90: return static_cast<MarkerType>(m_remainingTypes & -m_remainingTypes); > xiaochengh, MSVC is throwing a compiler warning on this line: > warning C4146: unary minus operator applied to unsigned type, result still unsigned > > I think m_remainingTypes should be signed but you said to make it unsigned, can I change it back? How about getting around it with |~m_remainingTypes + 1|?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2017/03/22 at 18:40:46, xiaochengh wrote: > On 2017/03/22 at 18:38:18, rlanday wrote: > > https://codereview.chromium.org/2765013003/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/markers/DocumentMarker.h (right): > > > > https://codereview.chromium.org/2765013003/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/markers/DocumentMarker.h:90: return static_cast<MarkerType>(m_remainingTypes & -m_remainingTypes); > > xiaochengh, MSVC is throwing a compiler warning on this line: > > warning C4146: unary minus operator applied to unsigned type, result still unsigned > > > > I think m_remainingTypes should be signed but you said to make it unsigned, can I change it back? > > How about getting around it with |~m_remainingTypes + 1|? Ok, updated
still lgtm
lgtm Thanks!
The CQ bit was checked by rlanday@chromium.org
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
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rlanday@chromium.org
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": 1490289666589470,
"parent_rev": "e0885fe729e75e0e692247e482907318b8960c42", "commit_rev":
"9b3e9af19463c051da0da835f4181bba204169a3"}
Message was sent while issue was closed.
Description was changed from ========== Add iterator for DocumentMarker::MarkerTypes This idea came up in code review for a DocumentMarkerController refactor: https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... ========== to ========== Add iterator for DocumentMarker::MarkerTypes This idea came up in code review for a DocumentMarkerController refactor: https://codereview.chromium.org/2723663002/diff/180002/third_party/WebKit/Sou... Review-Url: https://codereview.chromium.org/2765013003 Cr-Commit-Position: refs/heads/master@{#459159} Committed: https://chromium.googlesource.com/chromium/src/+/9b3e9af19463c051da0da835f418... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9b3e9af19463c051da0da835f418... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
