Need C++11 move semantics in the generated code

[os12]

It is 2017 and the protobuf-generated code needs the following C++11 features:

• Move constructor for every generated message
• Move assignment

The patch is trivial, see PR #2778. Yet this thing seems to be stuck due to political reasons... Opening an issue to track the progress.

[kevin-chau]

see #2780

[haberman]

We are currently experimenting with this inside Google and evaluating the effects on binary size, compile times, runtime performance, etc. This isn't political so much as technical; we need to ensure that this doesn't have adverse effects when deployed at scale across our large code base.

If these experiments go well, this change should show up in one of our merges from our internal code-base.

[os12]

Perhaps stating the obvious:

• my request is about generating C++ code and that has nothing to do with how protoc is written
• so you could just add --with-c++11 option to protoc and a corresponding option to configure which makes that default for the given installation

This will appease both camps - the ones who use the modern compiles as well as the rest, that are stuck with old distros.

[xfxyjwf]

The plan is to have protoc generate movable constructors by default (guarded by #ifdef CXX11). We are already experimenting this inside Google and I would rather not take a detour here.

For options, we are generally against them because they complicate our code base, build scripts and more importantly complicate user API/interface. Options also tend to cause interoperability problems. For example, what if an user uses a protoc built with --with-c++11 but depends on a protobuf library built without this --with-c++11 flag? It really doesn't worth the trouble to add options for a feature that we will soon have anyway.

[os12]

BTW, in addition to what I initially did in #2778, the generated code needs the following:

• move assignment for the generated message classes
• move-aware setter functions for the string fields

[xfxyjwf]

@os12 Move-aware setters for string fields are already there:
https://github.com/google/protobuf/blob/master/src/google/protobuf/compiler/cpp/cpp_string_field.cc#L885

The next release 3.4.0 will make generated proto messages movable.

[arthur-tacca]

I notice this has landed in the 3.4 branch. Thanks!

I noticed a little problem though: there are move constructors for generated messages, but not for message containers (RepeatedField, RepeatedPtrFieldBase, RepeatedPtrField<T>). It would be useful to have these too.

With the original proposal there was concern about code bloat because there's a move constructor and assignment for every generated message type, which there can be many of in an application. This worry doesn't apply to the containers, because we're just talking about four non-templated functions (move constructor and move assignment for the two classes), possibly inlined, plus two templated functions that are only instantiated if they're used.

[arthur-tacca]

I've realised another problem: The move constructor is not declared noexcept. I believe this means that std::vector will copy items rather than move them when resizing (including due to push_back etc.). It would also be sensible to declare move assignment operators as noexcept, although I don't know of any consequences of not doing that.

Of course the workarounds for this are the same as before move constructors were introduced: replace std::vector<MyMsgType> with std::vector<std::unique_ptr<MyMsgType>> or google::protobuf::RepeatedPtrField<MyMsgType>.

[denisiussion]

What about such C++11 feature as 'enum class' ?

[xfxyjwf]

@arthur-tacca @denisiussion Can you create separate issues for RepeatedField/noexcept/enum class?

[arthur-tacca]

@xfxyjwf Apologies that I never made an issue for move constructors/assignment for RepeatedField and co. I notice someone has added them anyway, so many thanks for that. 👍