From cdf3e81ff49b200213d67d65558f2919222b60ab Mon Sep 17 00:00:00 2001 From: Raphael Kubo da Costa Date: Mon, 16 Dec 2019 11:39:11 +0000 Subject: [PATCH] BookmarkModelMerger: Move RemoteTreeNode declaration to header. This fixes the build with libstdc++ after commit 8f5dad93e58 ("Fix CHECK failure due to untracked local nodes"): /usr/lib/gcc/x86_64-redhat-linux/9/../../../../include/c++/9/bits/stl_pair.h:215:11: error: field has incomplete type 'sync_bookmarks::BookmarkModelMerger::RemoteTreeNode' _T2 second; /// @c second is a copy of the second object ^ /usr/lib/gcc/x86_64-redhat-linux/9/../../../../include/c++/9/ext/aligned_buffer.h:91:28: note: in instantiation of template class 'std::pair, sync_bookmarks::BookmarkModelMerger::RemoteTreeNode>' requested here : std::aligned_storage ^ /usr/lib/gcc/x86_64-redhat-linux/9/../../../../include/c++/9/bits/hashtable_policy.h:233:43: note: in instantiation of template class '__gnu_cxx::__aligned_buffer, sync_bookmarks::BookmarkModelMerger::RemoteTreeNode> >' requested here __gnu_cxx::__aligned_buffer<_Value> _M_storage; ^ /usr/lib/gcc/x86_64-redhat-linux/9/../../../../include/c++/9/bits/hashtable_policy.h:264:39: note: in instantiation of template class 'std::__detail::_Hash_node_value_base, sync_bookmarks::BookmarkModelMerger::RemoteTreeNode> >' requested here struct _Hash_node<_Value, true> : _Hash_node_value_base<_Value> ^ /usr/lib/gcc/x86_64-redhat-linux/9/../../../../include/c++/9/bits/hashtable_policy.h:2028:25: note: in instantiation of template class 'std::__detail::_Hash_node, sync_bookmarks::BookmarkModelMerger::RemoteTreeNode>, true>' requested here rebind_traits; ^ /usr/lib/gcc/x86_64-redhat-linux/9/../../../../include/c++/9/bits/hashtable.h:184:15: note: in instantiation of template class 'std::__detail::_Hashtable_alloc, sync_bookmarks::BookmarkModelMerger::RemoteTreeNode>, true> > > ' requested here private __detail::_Hashtable_alloc< ^ /usr/lib/gcc/x86_64-redhat-linux/9/../../../../include/c++/9/bits/unordered_map.h:105:18: note: in instantiation of template class 'std::_Hashtable, std::pair, sync_bookmarks::BookmarkModelMerger::RemoteTreeNode>, std::allocator, sync_bookmarks::BookmarkModelMerger::RemoteTreeNode> >, std::__detail::_Select1st, std::equal_to >, std::hash, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__deta il::_Hashtable_traits >' requested here _Hashtable _M_h; ^ ../../components/sync_bookmarks/bookmark_model_merger.h:146:22: note: in instantiation of template class 'std::unordered_map, sync_bookmarks::BookmarkModelMerger::RemoteTreeNode, std::hash, std::equal_to >, std::allocator, sync_bookmarks::BookmarkModelMerger::RemoteTreeNode> > >' requested here const RemoteForest remote_forest_; ^ ../../components/sync_bookmarks/bookmark_model_merger.h:53:9: note: forward declaration of 'sync_bookmarks::BookmarkModelMerger::RemoteTreeNode' class RemoteTreeNode; ^ Essentially, the problem is that libstdc++'s std::unordered_map implementation requires both T and U to be fully declared. I raised the problem in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92770, and GCC's position is that we are relying on undefined behavior according to the C++ standard (https://eel.is/c++draft/requirements#res.on.functions-2.5). Bug: 957519 Change-Id: Ife7e435e516932a795bfbe05b2c910c3272878f0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1960156 Commit-Queue: Raphael Kubo da Costa Reviewed-by: Mikel Astiz Auto-Submit: Raphael Kubo da Costa Cr-Commit-Position: refs/heads/master@{#725070} --- .../sync_bookmarks/bookmark_model_merger.cc | 89 +++++++------------ .../sync_bookmarks/bookmark_model_merger.h | 48 +++++++++- 2 files changed, 80 insertions(+), 57 deletions(-) diff --git a/components/sync_bookmarks/bookmark_model_merger.cc b/components/sync_bookmarks/bookmark_model_merger.cc index eae153eff95..579848ee664 100644 --- a/components/sync_bookmarks/bookmark_model_merger.cc +++ b/components/sync_bookmarks/bookmark_model_merger.cc @@ -5,7 +5,6 @@ #include "components/sync_bookmarks/bookmark_model_merger.h" #include -#include #include #include #include @@ -205,66 +204,44 @@ UpdatesPerParentId GroupValidUpdatesByParentId( } // namespace -class BookmarkModelMerger::RemoteTreeNode final { - public: - // Constructs a tree given |update| as root and recursively all descendants by - // traversing |*updates_per_parent_id|. |update| and |updates_per_parent_id| - // must not be null. All updates |*updates_per_parent_id| must represent valid - // updates. Updates corresponding from descendant nodes are moved away from - // |*updates_per_parent_id|. - static RemoteTreeNode BuildTree( - std::unique_ptr update, - UpdatesPerParentId* updates_per_parent_id); - - ~RemoteTreeNode() = default; - - // Allow moves, useful during construction. - RemoteTreeNode(RemoteTreeNode&&) = default; - RemoteTreeNode& operator=(RemoteTreeNode&&) = default; - - const syncer::EntityData& entity() const { return *update_->entity; } - int64_t response_version() const { return update_->response_version; } - - // Direct children nodes, sorted by ascending unique position. These are - // guaranteed to be valid updates (e.g. IsValidBookmarkSpecifics()). - const std::vector& children() const { return children_; } - - // Recursively emplaces all GUIDs (this node and descendants) into - // |*guid_to_remote_node_map|, which must not be null. - void EmplaceSelfAndDescendantsByGUID( - std::unordered_map* - guid_to_remote_node_map) const { - DCHECK(guid_to_remote_node_map); - - const std::string& guid = entity().specifics.bookmark().guid(); - if (!guid.empty()) { - DCHECK(base::IsValidGUID(guid)); - - // Duplicate GUIDs have been sorted out before. - bool success = guid_to_remote_node_map->emplace(guid, this).second; - DCHECK(success); - } +BookmarkModelMerger::RemoteTreeNode::RemoteTreeNode() = default; - for (const RemoteTreeNode& child : children_) { - child.EmplaceSelfAndDescendantsByGUID(guid_to_remote_node_map); - } - } +BookmarkModelMerger::RemoteTreeNode::~RemoteTreeNode() = default; + +BookmarkModelMerger::RemoteTreeNode::RemoteTreeNode( + BookmarkModelMerger::RemoteTreeNode&&) = default; +BookmarkModelMerger::RemoteTreeNode& BookmarkModelMerger::RemoteTreeNode:: +operator=(BookmarkModelMerger::RemoteTreeNode&&) = default; + +void BookmarkModelMerger::RemoteTreeNode::EmplaceSelfAndDescendantsByGUID( + std::unordered_map* + guid_to_remote_node_map) const { + DCHECK(guid_to_remote_node_map); + + const std::string& guid = entity().specifics.bookmark().guid(); + if (!guid.empty()) { + DCHECK(base::IsValidGUID(guid)); - private: - static bool UniquePositionLessThan(const RemoteTreeNode& lhs, - const RemoteTreeNode& rhs) { - const syncer::UniquePosition a_pos = - syncer::UniquePosition::FromProto(lhs.entity().unique_position); - const syncer::UniquePosition b_pos = - syncer::UniquePosition::FromProto(rhs.entity().unique_position); - return a_pos.LessThan(b_pos); + // Duplicate GUIDs have been sorted out before. + bool success = guid_to_remote_node_map->emplace(guid, this).second; + DCHECK(success); } - RemoteTreeNode() = default; + for (const RemoteTreeNode& child : children_) { + child.EmplaceSelfAndDescendantsByGUID(guid_to_remote_node_map); + } +} - std::unique_ptr update_; - std::vector children_; -}; +// static +bool BookmarkModelMerger::RemoteTreeNode::UniquePositionLessThan( + const RemoteTreeNode& lhs, + const RemoteTreeNode& rhs) { + const syncer::UniquePosition a_pos = + syncer::UniquePosition::FromProto(lhs.entity().unique_position); + const syncer::UniquePosition b_pos = + syncer::UniquePosition::FromProto(rhs.entity().unique_position); + return a_pos.LessThan(b_pos); +} // static BookmarkModelMerger::RemoteTreeNode diff --git a/components/sync_bookmarks/bookmark_model_merger.h b/components/sync_bookmarks/bookmark_model_merger.h index 9b592000dc5..bf0783ecf8e 100644 --- a/components/sync_bookmarks/bookmark_model_merger.h +++ b/components/sync_bookmarks/bookmark_model_merger.h @@ -5,6 +5,7 @@ #ifndef COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_MODEL_MERGER_H_ #define COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_MODEL_MERGER_H_ +#include #include #include #include @@ -50,7 +51,52 @@ class BookmarkModelMerger { private: // Internal representation of a remote tree, composed of nodes. - class RemoteTreeNode; + class RemoteTreeNode final { + private: + using UpdatesPerParentId = + std::unordered_map; + + public: + // Constructs a tree given |update| as root and recursively all descendants + // by traversing |*updates_per_parent_id|. |update| and + // |updates_per_parent_id| must not be null. All updates + // |*updates_per_parent_id| must represent valid updates. Updates + // corresponding from descendant nodes are moved away from + // |*updates_per_parent_id|. + static RemoteTreeNode BuildTree( + std::unique_ptr update, + UpdatesPerParentId* updates_per_parent_id); + + ~RemoteTreeNode(); + + // Allow moves, useful during construction. + RemoteTreeNode(RemoteTreeNode&&); + RemoteTreeNode& operator=(RemoteTreeNode&&); + + const syncer::EntityData& entity() const { return *update_->entity; } + int64_t response_version() const { return update_->response_version; } + + // Direct children nodes, sorted by ascending unique position. These are + // guaranteed to be valid updates (e.g. IsValidBookmarkSpecifics()). + const std::vector& children() const { return children_; } + + // Recursively emplaces all GUIDs (this node and descendants) into + // |*guid_to_remote_node_map|, which must not be null. + void EmplaceSelfAndDescendantsByGUID( + std::unordered_map* + guid_to_remote_node_map) const; + + private: + static bool UniquePositionLessThan(const RemoteTreeNode& lhs, + const RemoteTreeNode& rhs); + + RemoteTreeNode(); + + std::unique_ptr update_; + std::vector children_; + }; // A forest composed of multiple trees where the root of each tree represents // a permanent node, keyed by server-defined unique tag of the root.