From 02d23ff991319761b9e5b99015b22b57206c8065 Mon Sep 17 00:00:00 2001
From: Otto <otto.moerbeek@open-xchange.com>
Date: Mon, 22 Feb 2021 12:27:47 +0100
Subject: [PATCH] Process review comments

---
 pdns/histogram.hh     | 32 +++++++++++++++-----------------
 pdns/pdns_recursor.cc |  2 +-
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/pdns/histogram.hh b/pdns/histogram.hh
index 34f429d7c6..5abb3c39d5 100644
--- a/pdns/histogram.hh
+++ b/pdns/histogram.hh
@@ -35,12 +35,6 @@ struct Bucket
   uint64_t d_count{0};
 };
 
-inline bool operator<(uint64_t b, const Bucket& bu)
-{
-  // we are using less-or-equal
-  return b <= bu.d_boundary;
-}
-
 struct AtomicBucket
 {
   // We need the constructors in this case, since atomics have a disabled copy constructor.
@@ -53,12 +47,6 @@ struct AtomicBucket
   std::atomic<uint64_t> d_count{0};
 };
 
-inline bool operator<(uint64_t b, const AtomicBucket& bu)
-{
-  // we are using less-or-equal
-  return b <= bu.d_boundary;
-}
-
 template<class B>
 class BaseHistogram
 {
@@ -75,12 +63,17 @@ public:
       throw std::invalid_argument("boundary array's first element should not be zero");
     }
     d_buckets.reserve(boundaries.size() + 1);
+    uint64_t prev = 0;
     for (auto b: boundaries) {
+      if (prev == b) {
+        throw std::invalid_argument("boundary array's elements should be distinct");
+      }
       std::string str = prefix + "le-" + std::to_string(b);
-      d_buckets.push_back(B{str, b, 0});
+      d_buckets.emplace_back(B{str, b, 0});
+      prev = b;
     }
     // everything above last boundary
-    d_buckets.push_back(B{prefix + "le-max", std::numeric_limits<uint64_t>::max(), 0});
+    d_buckets.emplace_back(B{prefix + "le-max", std::numeric_limits<uint64_t>::max(), 0});
   }
 
   const std::vector<B>& getRawData() const
@@ -100,7 +93,7 @@ public:
     uint64_t c{0};
     for (const auto& b : d_buckets) {
       c += b.d_count;
-      ret.push_back(B{b.d_name, b.d_boundary, c});
+      ret.emplace_back(B{b.d_name, b.d_boundary, c});
     }
     return ret;
   }
@@ -112,14 +105,19 @@ public:
     uint64_t c = 0;
     for (const auto& b : d_buckets) {
       c += b.d_count;
-      ret.push_back(c);
+      ret.emplace_back(c);
     }
     return ret;
   }
 
+  static bool lessOrEqual(uint64_t b, const B& bu)
+  {
+    return b <= bu.d_boundary;
+  }
+
   inline void operator()(uint64_t d)
   {
-    auto index = std::upper_bound(d_buckets.begin(), d_buckets.end(), d);
+    auto index = std::upper_bound(d_buckets.begin(), d_buckets.end(), d, lessOrEqual);
     // out index is always valid
     ++index->d_count;
   }
diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index 81baa50558..c72ed53641 100644
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -2143,7 +2143,7 @@ static void startDoResolve(void *p)
     }
 
     // Originally this code used a mix of floats, doubles, uint64_t with different units.
-    // Now it always uses an integral number of microseconds, expect for avarages, which are using doubles
+    // Now it always uses an integral number of microseconds, except for averages, which use doubles
     uint64_t spentUsec = uSec(sr.getNow() - dc->d_now);
     if (!g_quiet) {
       g_log<<Logger::Error<<t_id<<" ["<<MT->getTid()<<"/"<<MT->numProcesses()<<"] answer to "<<(dc->d_mdp.d_header.rd?"":"non-rd ")<<"question '"<<dc->d_mdp.d_qname<<"|"<<DNSRecordContent::NumberToType(dc->d_mdp.d_qtype);
-- 
2.47.2