[GitHub] [datasketches-cpp] jmalkin commented on a diff in pull request #269: Serde instance

[GitBox <gi...@apache.org>]

jmalkin commented on code in PR #269:
URL: https://github.com/apache/datasketches-cpp/pull/269#discussion_r851424243


##########
kll/include/kll_sketch_impl.hpp:
##########
@@ -334,8 +334,8 @@ double kll_sketch<T, C, S, A>::get_normalized_rank_error(bool pmf) const {
 
 // implementation for fixed-size arithmetic types (integral and floating point)
 template<typename T, typename C, typename S, typename A>
-template<typename TT, typename std::enable_if<std::is_arithmetic<TT>::value, int>::type>
-size_t kll_sketch<T, C, S, A>::get_serialized_size_bytes() const {
+template<typename TT, typename SerDe, typename std::enable_if<std::is_arithmetic<TT>::value, int>::type>
+size_t kll_sketch<T, C, S, A>::get_serialized_size_bytes(const SerDe&) const {

Review Comment:
   it doesn't seem like we're actually using the serde here anywhere?



##########
sampling/include/var_opt_union_impl.hpp:
##########
@@ -279,7 +294,7 @@ std::vector<uint8_t, AllocU8<A>> var_opt_union<T,S,A>::serialize(unsigned header
     ptr += copy_to_mem(outer_tau_numer_, ptr);
     ptr += copy_to_mem(outer_tau_denom_, ptr);
 
-    auto gadget_bytes = gadget_.serialize();
+    auto gadget_bytes = gadget_.serialize(header_size_bytes, sd);

Review Comment:
   i don't think we need to pass the header size to the gadget serializer here? seems like it'd just be the outer serialized container that needs it?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org