add missing check for MAX_NUM_ARRAY_ELEMENTS in the std::array pack/unpack. Also c++20 syntax update.

[greg7mdp]

Add missing check for MAX_NUM_ARRAY_ELEMENTS in the std::array versions.
Also c++20 syntax update.

Resolves #1422.

[spoonincode]

It might be nice, since you're touching them anyways, to update these to c++20 syntax. but obviously not critical and can go as-is

[greg7mdp]

It might be nice, since you're touching them anyways, to update these to c++20 syntax. but obviously not critical and can go as-is

Hopefully this is what you were thinking of.

[greg7mdp]

Upon reflection, I don't think std::is_trivially_copyable is appropriate.

• pointers are std::trivially_copyable. We wouldn't want to allow serializing an std::array of pointers.
• we may have a type for which FC_REFLECT was defined, which we serialize without alignment padding
  between members. But that type can also be std::trivially_copyable, and we wouldn't want to
  serialize it with padding when in a std::array I think.

[greg7mdp]

@heifner @spoonincode I made some changes in this PR (realized std::is_trivially_copyable was not an appropriate replacement), and updated the PR and issue descriptions. Maybe you guys would like to have another look?

[heifner]

My understanding is std::array can have padding because it is a struct. However we don't serialize the std::array we serialize the internal c-array of the std::array. So padding is not an issue.

[spoonincode]

My understanding is std::array can have padding because it is a struct. However we don't serialize the std::array we serialize the internal c-array of the std::array. So padding is not an issue.

I think the problem is if someone has

struct foo {
   uint32_t bar;
   uint64_t baz;
};
FC_REFLECT(foo, (bar)(baz));

This is a trivially copyable type (std::is_trivially_copyable), but has padding. So if someone makes an array<foo, 2> we can't just write out the bytes of the entire array (like early change in this PR would have done).

I wonder how we could detect that something like sha256 is safe to handle in this manner.

[greg7mdp]

My understanding is std::array can have padding because it is a struct. However we don't serialize the std::array we serialize the internal c-array of the std::array. So padding is not an issue.

Hum, that's not exactly what I meant. Consider struct key_weight, which has these members:

spring/libraries/chain/include/eosio/chain/authority.hpp

Lines 95 to 97 in 25978a6

	struct key_weight {
	public_key_type key;
	weight_type weight;

A std::array<key_weight> will have space paddings between the key_weight values, since weight_type is an uint16_t.
But it may be std::is_trivially_copyable, so we would write the memory representation of the array, spaces and all.

So that would be a consensus change from what we were writing before, as key_weight would not have been is_trivial_array, and we would have serialized the values individually without the padding between values.

Same issue if the members were in the opposite order, weight before key. We'd have internal padding between the two members, which would be written if we just write the memory representation of the std::array, but not if we write the values separately using:

spring/libraries/chain/include/eosio/chain/authority.hpp

Line 362 in 25978a6

FC_REFLECT(eosio::chain::key_weight, (key)(weight) )

[greg7mdp]

I wonder how we could detect that something like sha256 is safe to handle in this manner.

I had the exact same thought! We could detect it if sha256 used a std::array (a std::array of std::is_scalar type would be OK), but since it is using a c-style array (which decays as pointer)

spring/libraries/libfc/include/fc/crypto/sha256.hpp

Line 108 in 25978a6

uint64_t _hash[4];

I don't think we can safely match it, but then I didn't quite get what this does:

spring/libraries/libfc/include/fc/crypto/sha256.hpp

Line 158 in 25978a6

FC_REFLECT_TYPENAME( fc::sha256 )

[heifner]

Thanks guys. I did misunderstand the issue at hand.

I don't think we can safely match it, but then I didn't quite get what this does:

spring/libraries/libfc/include/fc/crypto/sha256.hpp

Line 158 in 25978a6

FC_REFLECT_TYPENAME( fc::sha256 )

That just flags that the type works with reflection. It specifies the operator<< & operator>> manually instead of using FC_REFLECT.

[greg7mdp]

That just flags that the type works with reflection. It specifies the operator<< & operator>> manually instead of using FC_REFLECT.

Oh, interesting, so it does write the four uint64_t in one shot. nice!