New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 860435 link

Starred by 2 users
Status: Fixed
Owner:
█ davidovic@google.com
Last visit > 30 days ago
Closed: Aug 17
Cc:
mastiz@chromium.org
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Task



Sign in to add a comment

Sync: Improve EnumSet's iterator interface

Project Member Reported by treib@chromium.org, Jul 5 
components/sync/base/enum_set.h contains a specialized implementation of a set of enums. That's arguably useful, but its current iterator interface is quite inconvenient to use. We should consider making it standard-iterator-compliant, so that it can be used in range-based for loops.

Example:
We could replace

for (EnumSet<EnumType>::Iterator it = enums.First(); it.Good(); it.Inc()) {
  DoStuff(it.Get());
}

with

for (EnumType value : enums) {
  DoStuff(value);
}

Note that a comment on EnumSet [1] claims that its iterator semantics are very different from standard iterators, but that doesn't seem to be true.

https://cs.chromium.org/chromium/src/components/sync/base/enum_set.h?rcl=dc7f5252145dbef0c1663176a90e86285d5e0e3b&l=55

Comment 1 by vitaliii@chromium.org, Jul 18

Owner: davidovic@google.com
Status: Started (was: Available)
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 16 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/75f9b72ea28ee6b59d93e9c022a423fa6b0a335f

commit 75f9b72ea28ee6b59d93e9c022a423fa6b0a335f
Author: David Davidović <davidovic@google.com>
Date: Thu Aug 16 19:16:11 2018

[sync::cleanup] Add STL iterator to EnumSet

Add an STL-compatible iterator interface to EnumSet in order to enable
range-based for loops and have a standard interface with which everybody
is familiar. This includes defining comparison, increment, and deref
operators in EnumSet::Iterator and providing EnumSet::begin and
EnumSet::end which behave in the same way as these methods do on STL
classes.

Bug:  860435 
Change-Id: If4312d5b2a1e282ac3510e22bcc29b3525cc43d9
Reviewed-on: https://chromium-review.googlesource.com/1167286
Commit-Queue: David Davidović <davidovic@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583765}
[modify] https://crrev.com/75f9b72ea28ee6b59d93e9c022a423fa6b0a335f/components/sync/base/enum_set.h
[modify] https://crrev.com/75f9b72ea28ee6b59d93e9c022a423fa6b0a335f/components/sync/base/enum_set_unittest.cc

Comment 3 by tschumann@chromium.org, Aug 16 

tbh, i have strong reservations against project-internal base classes as they tend to over generalize.

EnumSet is a good example: it has exactly 1 use, FullModelTypeSet:
https://cs.chromium.org/search/?q=EnumSet+file:%5Esrc/components/sync/+package:%5Echromium$&type=cs

and FullModelTypeSet in turn is used in only 7 files:
https://cs.chromium.org/search/?q=FullModelTypeSet+f:sync&type=cs

I'd rather remove that rarely used abstraction and replace FullModelTypeSet by a non templatized class using std:set and min/max getters.

My 0.02$, no need to take action right away.

Comment 4 by davidovic@google.com, Aug 16 

The most widely used specialization of EnumSet is ModelTypeSet: https://cs.chromium.org/chromium/src/components/sync/base/model_type.h?type=cs&sq=package:chromium&g=0&l=163. ModelTypeSet, in turn, is used considerably across the Sync codebase: https://cs.chromium.org/search/?q=modeltypeset&type=cs.

EnumSet is still only ever specialized with ModelType as the enum, so I feel that your point still stands. But I don't feel good about the alternatives, either. std::set/std::unordered_set would add a fair bit of overhead over EnumSet (which is essentially std::bitset), and using a raw std::bitset would concede the type-safety that EnumSet provides.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 16 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0461846a466628695233cbd03a4d16b10138366

commit f0461846a466628695233cbd03a4d16b10138366
Author: David Davidović <davidovic@google.com>
Date: Thu Aug 16 21:02:51 2018

[sync::cleanup] Remove old EnumSet iter uses (1/2)

After https://chromium-review.googlesource.com/c/chromium/src/+/1145066,
which adds an STL-compatible interface to EnumSet and EnumSet::Iterator,
replace all uses of the previous iteration interface with a range-based
for loop. This allows eventual removal of the old interface altogether.

Also, replace existing usage of |arraysize| with |base::Size|, since the
former was deprecated.

This is CL part 1 of 2.

Bug:  860435 
Change-Id: Ida43a641fd24f567be162ae5e18665e2ea7e39f6
Reviewed-on: https://chromium-review.googlesource.com/1146819
Commit-Queue: David Davidović <davidovic@google.com>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583808}
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/base/invalidation_helper.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/base/sync_prefs.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/base/sync_prefs_unittest.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/driver/data_type_manager_impl.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/driver/data_type_manager_impl_unittest.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/driver/data_type_status_table.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/driver/generic_change_processor_unittest.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/driver/glue/sync_backend_host_core.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/driver/model_association_manager.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine/engine_components_factory_impl.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine/fake_sync_manager.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine/sync_backend_registrar.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/apply_control_data_updates.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/commit_processor.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/commit_util.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/cycle/nudge_tracker.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/cycle/nudge_tracker_unittest.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/debug_info_event_listener.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/get_updates_processor.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/get_updates_processor_unittest.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/loopback_server/loopback_server.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/model_type_registry_unittest.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/sync_encryption_handler_impl.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/sync_manager_for_profile_sync_test.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/sync_manager_impl.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/sync_manager_impl_unittest.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/sync_scheduler_impl.cc
[modify] https://crrev.com/f0461846a466628695233cbd03a4d16b10138366/components/sync/engine_impl/sync_scheduler_impl_unittest.cc

Comment 6 by tschumann@chromium.org, Aug 17 

hah, somehow missed ModelTypeSet. In that case the class is as at least heavily used. Still not sure if it's worth so much generalized template code, but changing that is not worth the effort -- let's just hope there are no bugs :D.
Sorry for the noise.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 17 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59865880a04b72d9e69626aa1a4593ed2295047a

commit 59865880a04b72d9e69626aa1a4593ed2295047a
Author: David Davidović <davidovic@google.com>
Date: Fri Aug 17 09:51:15 2018

[sync::cleanup] Remove old EnumSet iter uses (2/2)

After https://chromium-review.googlesource.com/c/chromium/src/+/1145066,
which adds an STL-compatible interface to EnumSet and EnumSet::Iterator,
replace all uses of the previous iteration interface with a range-based
for loop. This allows eventual removal of the old interface altogether.

Also, replace existing usage of |arraysize| with |base::Size|, since the
former was deprecated.

This is CL part 2 of 2.

TBR=dschuyler@chromium.org

Bug:  860435 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I7839362e3b103e9a1d309f21c7b1bef17da7b8e1
Reviewed-on: https://chromium-review.googlesource.com/1146925
Commit-Queue: David Davidović <davidovic@google.com>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584006}
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/chrome/browser/sync/profile_sync_service_android.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/chrome/browser/sync/test/integration/enable_disable_test.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/chrome/browser/sync/test/integration/migration_test.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/chrome/browser/sync/test/integration/quiesce_status_change_checker.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/chrome/browser/sync/test/integration/sync_test.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/chrome/browser/ui/webui/settings/people_handler_unittest.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/chrome/browser/ui/webui/sync_internals_message_handler.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/components/sync/syncable/directory.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/components/sync/syncable/directory_backing_store.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/components/sync/syncable/directory_backing_store_unittest.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/components/sync/syncable/directory_unittest.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/components/sync/syncable/model_type.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/components/sync/syncable/model_type_unittest.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/components/sync/syncable/syncable_write_transaction.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/components/sync/test/engine/mock_connection_manager.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/components/sync/tools/sync_client.cc
[modify] https://crrev.com/59865880a04b72d9e69626aa1a4593ed2295047a/ios/chrome/browser/ui/webui/sync_internals/sync_internals_message_handler.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 17 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/64a6592b3f9dcd037549c1d55e728b305f3cb379

commit 64a6592b3f9dcd037549c1d55e728b305f3cb379
Author: David Davidović <davidovic@google.com>
Date: Fri Aug 17 10:54:16 2018

[sync::cleanup] Remove old EnumSet iter interface

https://chromium-review.googlesource.com/c/chromium/src/+/1145066 adds a
new STL-compatible interface to EnumSet and EnumSet::Iterator.

https://chromium-review.googlesource.com/c/chromium/src/+/1145182 and
https://chromium-review.googlesource.com/c/chromium/src/+/1145186 completely
remove usage of the old one.

Due to this, the old interface is not needed anymore.

This refers to the methods EnumSet::First (superseded by EnumSet::begin),
EnumSet::Iterator::Get (superseded by unary operator * for dereferencing) and
EnumSet::Iterator::Inc (superseded by unary operator ++ for incrementing).
Therefore, remove them. EnumSet::Iterator::Good remains because it is used
internally, but its visibility has been changed from public to private to
reflect this.

Bug:  860435 
Change-Id: I7d27123cea02701795b6b9d278a3d9556fefc697
Reviewed-on: https://chromium-review.googlesource.com/1146929
Commit-Queue: David Davidović <davidovic@google.com>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584019}
[modify] https://crrev.com/64a6592b3f9dcd037549c1d55e728b305f3cb379/components/sync/base/enum_set.h
[modify] https://crrev.com/64a6592b3f9dcd037549c1d55e728b305f3cb379/components/sync/base/enum_set_unittest.cc

Comment 9 by davidovic@google.com, Aug 17

Status: Fixed (was: Started)
Thanks everyone for the input and reviews. This has landed completely: the interface has been replaced with the new one and all occurrences in the codebase have been migrated accordingly. Closing.
►

Sign in to add a comment