Remove fc::array and replace usage with std::array
#153
Conversation
…y and removes some unneeded copies.
heifner
force-pushed
the
feature/rm-fc-array
branch
from
7d41b5d to
6b7d5e9
Compare
jglanz
left a comment
jglanz
left a comment
There was a problem hiding this comment.
Read the comments (non-nit) for feedback before merge
|
|
||
| #include <sysio/chain/types.hpp> | ||
| #include <fc/io/raw.hpp> | ||
| #include <fc/crypto/base64.hpp> |
There was a problem hiding this comment.
nit: Double check the header is needed as nothing else was added to the header?
There was a problem hiding this comment.
fc/array.hpp included fc/crypto/base64.hpp so it was pulled in to many files.
base64_encode and base64_decode are used in to_variant / from_variant.
| inline ostream_wrapper& | ||
| operator<<(ostream_wrapper& ds, const fc::crypto::ed::public_key_shim& pk) { | ||
| ds.write(reinterpret_cast<const char*>(pk._data.data), crypto_sign_PUBLICKEYBYTES); | ||
| ds.write(reinterpret_cast<const char*>(pk._data.data()), crypto_sign_PUBLICKEYBYTES); | ||
| return ds; | ||
| } | ||
|
|
||
| inline ostream_wrapper& | ||
| operator<<(ostream_wrapper& ds, const fc::crypto::ed::signature_shim& sig) { | ||
| ds.write(reinterpret_cast<const char*>(sig._data.data), crypto_sign_BYTES); | ||
| ds.write(reinterpret_cast<const char*>(sig._data.data()), crypto_sign_BYTES); | ||
| return ds; | ||
| } |
There was a problem hiding this comment.
This is not needed at all. Removed.
| static bls12_381::g2 to_jacobian_montgomery_le(const bls::signature_data& affine_non_montgomery_le); | ||
| private: | ||
| bls::signature_data::std_array_type _affine_non_montgomery_le{}; | ||
| bls::signature_data _affine_non_montgomery_le{}; |
There was a problem hiding this comment.
should these be const as they are only set in constructor?
There was a problem hiding this comment.
They have to be non-const for reflection
| #include <concepts> | ||
| #include <iterator> | ||
| #include <ranges> | ||
| #include <type_traits> | ||
| #include <cstring> |
There was a problem hiding this comment.
nit: move std include group above fc
There was a problem hiding this comment.
I disagree. I think std should be last.
| concept SerializableForMemcmp = requires(const T& obj) { | ||
| { obj.serialize() } -> std::ranges::forward_range; | ||
| { obj.serialize().data() } -> std::contiguous_iterator; // Ensures data is contiguous for memcmp | ||
| { obj.serialize().size() } -> std::convertible_to<std::size_t>; | ||
| } && std::is_trivially_copyable_v<std::ranges::range_value_t<decltype(std::declval<const T&>().serialize())>>; |
There was a problem hiding this comment.
Nice. Should/could this be generified and moved to something like (key|crypto)_traits.hpp?
There was a problem hiding this comment.
Already in common.hpp and is rather specific to serialize() method.
Normalizes to
std::arrayand removes some unneeded copies.Maintains same serialization and comparison.