Correct object copying in document.h

[ns-osmolsky]

• this came up when building the code with GCC9
  • copying C++ objects with memcpy() is only permitted for Trivial types, the rest
    should be copied via copy or assignment
  • tweaked the code to construct the objects in place and then assign

Ran "make test", looks OK.

Fixes #1700

[ns-osmolsky]

Wow, it looks like the old Visual Studio 2013 does not understand = default.

[miloyip]

=default is C++11. Need C++03.

[miloyip]

I am not sure if this change would affect runtime performance.

[ns-osmolsky]

OK, I've just pushed a C++03 workaround.

As for perf, I ran the tests on my VM and don't see any difference.

[coveralls]

Coverage decreased (-0.3%) to 99.587% when pulling 6c812e0 on ns-osmolsky:master into 8f4c021 on Tencent:master.

[miloyip]

But I think it may have performance overhead, for example, default construction and assignment is non-trivial. Default construction sets type to kNullType, and then assignment needs to first checking the type and deallocate it if needed.

[ns-osmolsky]

There is definitely overhead, yet it may be insignificant for the types in play here. Do you have a concrete benchmark that you are concerned with?

Or looking at it from the opposite perspective, this change is about correctness. Doing memcpy() on complex C++ objects is crazy.

[miloyip]

GenericValue is not really complex object at all. It is not polymorphic. It does not contains non-trival member variables. Actually it is designed for possibility of memcpy.
I don't think it is "incorrect".

[ns-osmolsky]

Please build with GCC9 and it will tell you that the code is incorrect... even if it happens to work in this special case. What you are doing results in undefined behavior as per the ISO C++ Standard.

[pah]

As usual, another possibility is to add a new level of indirection and put all members into a trivial local struct and memcpy that instead of the whole object. This will satisfy the C++ standard and achieves the same behaviour as before.

Edit: Ok, this is not as straight-forward as we keep real objects in the array/object buffers.

[ns-osmolsky]

s usual, another possibility is to add a new level of indirection and put all members into a trivial local struct and memcpy that instead of the whole object. This will satisfy the C++ standard and achieves the same behaviour as before.

Well, yet that would still involve construction for each object (as I am doing already) followed by a memcpy of a member (while I am doing copy assignment of the whole).

The net result is looks identical to me.

[ns-osmolsky]

Closing the PR as it is just hanging here:

• I've made this change in my own fork
• GCC9 break: trying to memcpy C++ objects #1700 remains open for tracking
• feel free to resurrect and merge

[kvtb]

the PR is merge error to the latest version, so it is better to keep it open and rebase from time to time