Integrate review feedback

src/OpaqueVal.cc

@@ -41,7 +41,7 @@ namespace zeek {
 // Helper to retrieve a broker count out of a list at a specified index, and
 // casted to the expected destination type.
 template<typename V, typename D>
-inline bool get_vector_idx(V& v, size_t i, D* dst) {
+bool get_vector_idx_if_count(V& v, size_t i, D* dst) {
     if ( i >= v.Size() || ! v[i].IsCount() )
         return false;
 
@@ -663,8 +663,10 @@ bool EntropyVal::Get(double* r_ent, double* r_chisq, double* r_mean, double* r_m
 IMPLEMENT_OPAQUE_VALUE(EntropyVal)
 
 std::optional<BrokerData> EntropyVal::DoSerialize() const {
+    constexpr size_t numMembers = 14; // RandTest has 14 non-array members.
+
     BrokerListBuilder builder;
-    builder.Reserve(256 + 3 + RT_MONTEN + 11);
+    builder.Reserve(numMembers + std::size(state.ccount) + std::size(state.monte));
 
     builder.Add(static_cast<uint64_t>(state.totalc));
     builder.Add(static_cast<uint64_t>(state.mp));
@@ -694,46 +696,46 @@ bool EntropyVal::DoUnserialize(BrokerDataView data) {
     if ( ! data.IsList() )
         return false;
 
+    auto index = size_t{0};
+
     auto d = data.ToList();
 
-    if ( ! get_vector_idx(d, 0, &state.totalc) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.totalc) )
         return false;
-    if ( ! get_vector_idx(d, 1, &state.mp) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.mp) )
         return false;
-    if ( ! get_vector_idx(d, 2, &state.sccfirst) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.sccfirst) )
         return false;
-    if ( ! get_vector_idx(d, 3, &state.inmont) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.inmont) )
         return false;
-    if ( ! get_vector_idx(d, 4, &state.mcount) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.mcount) )
         return false;
-    if ( ! get_vector_idx(d, 5, &state.cexp) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.cexp) )
         return false;
-    if ( ! get_vector_idx(d, 6, &state.montex) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.montex) )
         return false;
-    if ( ! get_vector_idx(d, 7, &state.montey) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.montey) )
         return false;
-    if ( ! get_vector_idx(d, 8, &state.montepi) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.montepi) )
         return false;
-    if ( ! get_vector_idx(d, 9, &state.sccu0) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.sccu0) )
         return false;
-    if ( ! get_vector_idx(d, 10, &state.scclast) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.scclast) )
         return false;
-    if ( ! get_vector_idx(d, 11, &state.scct1) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.scct1) )
         return false;
-    if ( ! get_vector_idx(d, 12, &state.scct2) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.scct2) )
         return false;
-    if ( ! get_vector_idx(d, 13, &state.scct3) )
+    if ( ! get_vector_idx_if_count(d, index++, &state.scct3) )
         return false;
 
-    auto index = size_t{14};
-
     for ( auto& bin : state.ccount ) {
-        if ( ! get_vector_idx(d, index++, &bin) )
+        if ( ! get_vector_idx_if_count(d, index++, &bin) )
             return false;
     }
 
     for ( auto& val : state.monte ) {
-        if ( ! get_vector_idx(d, index++, &val) )
+        if ( ! get_vector_idx_if_count(d, index++, &val) )
             return false;
     }
 
@@ -1036,7 +1038,7 @@ bool ParaglobVal::DoUnserialize(BrokerDataView data) {
     iv->resize(d.Size());
 
     for ( size_t index = 0; index < d.Size(); ++index ) {
-        if ( ! get_vector_idx(d, index, iv->data() + index) )
+        if ( ! get_vector_idx_if_count(d, index, iv->data() + index) )
             return false;
     }
 

src/broker/Data.h

@@ -406,7 +406,7 @@ private:
  * Convenience function to check whether a list of Broker data values are all of type `count`.
  */
 template<typename... Args>
-[[nodiscard]] inline bool IsCount(BrokerDataView arg, Args&&... args) {
+[[nodiscard]] bool are_all_counts(BrokerDataView arg, Args&&... args) {
     return arg.IsCount() && (args.IsCount() && ...);
 }
 
@@ -414,7 +414,7 @@ template<typename... Args>
  * Convenience function to check whether a list of Broker data values are all of type `integer`.
  */
 template<typename... Args>
-[[nodiscard]] inline auto ToCount(BrokerDataView arg, Args&&... args) {
+[[nodiscard]] auto to_count(BrokerDataView arg, Args&&... args) {
     return std::tuple{arg.ToCount(), args.ToCount()...};
 }
 
@@ -557,22 +557,32 @@ public:
      */
     template<typename T>
     void AddCount(T value) {
+        if constexpr ( std::is_enum_v<T> ) {
+            AddCount(static_cast<std::underlying_type_t<T>>(value));
+        }
+        else {
             static_assert(std::is_integral_v<T> && ! std::is_same_v<bool, T>);
             static_assert(std::is_unsigned_v<T>);
             static_assert(sizeof(T) <= sizeof(broker::count));
             values_.emplace_back(static_cast<broker::count>(value));
         }
+    }
 
     /**
      * Adds `value` as a Broker `integer` to the list, automatically converting it if necessary.
      */
     template<typename T>
     void AddInteger(T value) {
+        if constexpr ( std::is_enum_v<T> ) {
+            AddInteger(static_cast<std::underlying_type_t<T>>(value));
+        }
+        else {
             static_assert(std::is_integral_v<T> && ! std::is_same_v<bool, T>);
             static_assert(std::is_signed_v<T>);
             static_assert(sizeof(T) <= sizeof(broker::integer));
             values_.emplace_back(static_cast<broker::integer>(value));
         }
+    }
 
     /**
      * Appends `value` to the end of the list.
@@ -604,7 +614,7 @@ public:
      * @param cstr The characters to append.
      * @param len The number of characters to append.
      */
-    void AddString(const char* cstr, size_t len) { values_.emplace_back(std::string{cstr, len}); }
+    void Add(const char* cstr, size_t len) { values_.emplace_back(std::string{cstr, len}); }
 
     /**
      * Appends `value` to the end of the list.

src/probabilistic/CardinalityCounter.cc

@@ -191,10 +191,10 @@ std::unique_ptr<CardinalityCounter> CardinalityCounter::Unserialize(BrokerDataVi
         return nullptr;
 
     auto v = data.ToList();
-    if ( v.Size() < 3 || ! IsCount(v[0], v[1]) || ! v[2].IsReal() )
+    if ( v.Size() < 3 || ! are_all_counts(v[0], v[1]) || ! v[2].IsReal() )
         return nullptr;
 
-    auto [m, V] = ToCount(v[0], v[1]);
+    auto [m, V] = to_count(v[0], v[1]);
     auto alpha_m = v[2].ToReal();
 
     if ( v.Size() != 3 + m )

src/probabilistic/Hasher.cc

@@ -61,10 +61,10 @@ std::unique_ptr<Hasher> Hasher::Unserialize(BrokerDataView data) {
 
     auto v = data.ToList();
 
-    if ( v.Size() != 4 || ! IsCount(v[0], v[1], v[2], v[3]) )
+    if ( v.Size() != 4 || ! are_all_counts(v[0], v[1], v[2], v[3]) )
         return nullptr;
 
-    auto [type, k, h1, h2] = ToCount(v[0], v[1], v[2], v[3]);
+    auto [type, k, h1, h2] = to_count(v[0], v[1], v[2], v[3]);
 
     std::unique_ptr<Hasher> hasher;
 

src/probabilistic/Topk.cc

@@ -423,6 +423,9 @@ bool TopkVal::DoUnserialize(BrokerDataView data) {
     bool ok = true;
     auto index = size_t{4}; // Index into v.
     auto atEnd = [&v, &index] { return index >= v.Size(); };
+    // Returns the element  at the given index in v, if that element is a count.
+    // If so, ok becomes true, and the index gets incremented.
+    // If not, ok becomes false, and the index remains unchanged.
     auto nextCount = [&v, &ok, &index]() -> uint64_t {
         if ( index >= v.Size() || ! v[index].IsCount() ) {
             ok = false;

src/script_opt/CPP/Func.cc

@@ -66,7 +66,7 @@ std::optional<BrokerData> CPPLambdaFunc::SerializeCaptures() const {
 
     BrokerListBuilder builder;
     builder.Reserve(2);
-    builder.AddString(name.data(), name.size());
+    builder.Add(name.data(), name.size());
     builder.Add(std::move(body));
     return std::move(builder).Build();
 }