Use MetadataChangeList and EntityChangeList in SharedModelTypeProcessor

sync/api/entity_change.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef SYNC_API_ENTITY_CHANGE_H_
 #define SYNC_API_ENTITY_CHANGE_H_
 
 #include <string>
 #include <vector>
 
 #include "sync/api/entity_data.h"
 #include "sync/base/sync_export.h"
 
 namespace syncer_v2 {
 
 class SYNC_EXPORT EntityChange {

[skym, 2016/01/07 00:40:48]

Need a comment explaining the purpose of this class.

  public:
   enum ChangeType {
     ACTION_ADD,
     ACTION_UPDATE,
     ACTION_DELETE
   };
 
   static EntityChange CreateAdd(std::string client_key, EntityDataPtr data);
   static EntityChange CreateUpdate(std::string client_key, EntityDataPtr data);
   static EntityChange CreateDelete(std::string client_key);
 
   virtual ~EntityChange();
 
   std::string client_key() const { return client_key_; }
   ChangeType type() const { return type_; }
   const EntityData& data() const { return data_.value(); }
 
  private:
   EntityChange(std::string client_key, ChangeType type, EntityDataPtr data);
 
-  const std::string client_key_;
-  const ChangeType type_;
-  const EntityDataPtr data_;
+  std::string client_key_;

[skym, 2016/01/07 00:40:48]

I don't quite follow why these need to be mutable based off the other changes in
this CL.

[stanisc, 2016/01/11 19:56:47]

The compiler complains that copy constructor is deleted when these fields are
consts. The copy constructor is used when inserting EntityChange into
EntityChangeList.

[skym, 2016/01/11 20:45:40]

That makes me so sad. It should be completely possible for a copy constructor to
set all these fields correctly. Maybe we should define it? The assignment
operator on the other hand, won't work here regardless, right? And I think
std::vector::push_back requires the type the be assignable.

If we made this type movable, I think it might actually solve this problem. I
think Chromium style has recently been updated to allow explicit move
constructors http://chromium-cpp.appspot.com/ . Also if this works, you could
throw in DISALLOW_COPY_AND_ASSIGN as well!

There's also the emplace_back option (although not currently allowed, presumably
this is going to change in the near future). However, since we're using a static
method and private constructor, we'd have to write a custom allocator, yuck.

[stanisc, 2016/01/12 19:32:11]

I think these fields have to be assignable because when an element is inserted
into a vector the vector is first expanded to a larger size, which involves
construction of new elements at the end, then the new element is copied over. So
essentially existing elements are assigned new values. I am not sure if we need
to further complicate that now.

[skym, 2016/01/12 19:38:54]

You're right, in order for EntityChange to be assignable, these fields must be
assignable. But one of the points I was trying to make was I think EntityChange
only needs to be either assignable or movable to work with std::vector. You make
a good point though, no need to deal with this now.

+  ChangeType type_;
+  EntityDataPtr data_;
 };
 
 typedef std::vector<EntityChange> EntityChangeList;
 
 }  // namespace syncer_v2
 
 #endif  // SYNC_API_ENTITY_CHANGE_H_