SampleSet -> HistogramSamples (will be reused by SparseHistogram)

base/metrics/histogram_samples.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/metrics/histogram_samples.h"
+
+#include "base/compiler_specific.h"
+#include "base/pickle.h"
+
+namespace base {
+
+namespace {
+
+class SampleCountPickleIterator : public SampleCountIterator {
+ public:
+  SampleCountPickleIterator(PickleIterator* iter);
+
+  virtual bool Done() OVERRIDE;
+  virtual void Next() OVERRIDE;
+  virtual void Get(HistogramBase::Sample* min,
+                   HistogramBase::Sample* max,
+                   HistogramBase::Count* count) OVERRIDE;
+
+ private:
+  bool ReadNext();
+
+  PickleIterator* iter_;
+
+  HistogramBase::Sample min_;
+  HistogramBase::Sample max_;
+  HistogramBase::Count count_;
+  bool is_done_;
+};
+
+SampleCountPickleIterator::SampleCountPickleIterator(PickleIterator* iter)
+    : iter_(iter), is_done_(false) {

[Ilya Sherman, 2012/08/23 07:50:54]

nit: Initializations should be one per line:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...

[kaiwang, 2012/08/24 04:17:58]

actually it only says subsequent lines should indent 4 spaces, but not
everything should be on same line...

Anyway, fixed with your suggestion

+  if (!ReadNext()) {
+    is_done_ = true;
+  }

[Ilya Sherman, 2012/08/23 07:50:54]

nit: No need for curly braces

[Ilya Sherman, 2012/08/23 07:50:54]

nit: Why not just call Next()?

[kaiwang, 2012/08/24 04:17:58]

Done.

[Ilya Sherman, 2012/08/29 08:48:16]

(bump)

[kaiwang, 2012/08/30 03:13:21]

Done.

+}
+
+bool SampleCountPickleIterator::Done() {
+  return is_done_;
+}
+
+void SampleCountPickleIterator::Next() {
+  CHECK(!Done());

[Ilya Sherman, 2012/08/23 07:50:54]

Does this really need to be a CHECK rather than a DCHECK?

[kaiwang, 2012/08/24 04:17:58]

Using DCHECK asks code owner to consider 2 situations: in release and in debug,
this complicates the code. And style guide does not suggest to "gracefully
handle DCHECK failure".

I think the only reason to use DCHECK is to prevent impacting performance

[Ilya Sherman, 2012/08/29 08:48:16]

The Chromium default is to use DCHECK rather than CHECK, both to avoid impacting
performance and to avoid impacting the binary size.  CHECK is only used when
DCHECK does not suffice for some reason, e.g. when you are trying to track down
crashes that seem to be violating your assertion.

+  if (!ReadNext()) {
+    is_done_ = true;
+  }
+}
+
+void SampleCountPickleIterator::Get(HistogramBase::Sample* min,
+                                    HistogramBase::Sample* max,
+                                    HistogramBase::Count* count) {
+  CHECK(!Done());

[Ilya Sherman, 2012/08/23 07:50:54]

Ditto

+  *min = min_;
+  *max = max_;
+  *count = count_;
+}
+
+bool SampleCountPickleIterator::ReadNext() {
+  if (!iter_->ReadInt(&min_) ||
+      !iter_->ReadInt(&max_) ||
+      !iter_->ReadInt(&count_)) {
+    return false;
+  }
+  return true;

[Ilya Sherman, 2012/08/23 07:50:54]

nit: This can be written as

return iter_->ReadInt(&min_) && iter_->ReadInt(&max) && iter_->ReadInt(&count_);

[kaiwang, 2012/08/24 04:17:58]

Done.

+}
+
+}  // namespace
+
+HistogramSamples::HistogramSamples()
+    : sum_(0), redundant_count_(0) {}

[Ilya Sherman, 2012/08/23 07:50:54]

nit: One per line

[kaiwang, 2012/08/24 04:17:58]

moved to a single line

+
+HistogramSamples::~HistogramSamples() {}
+
+void HistogramSamples::Add(const HistogramSamples& other) {
+  sum_ += other.sum();
+  redundant_count_ += other.redundant_count();
+  CHECK(AddSubtractHelper(other.Iterator().get(), true));

[Ilya Sherman, 2012/08/23 07:50:54]

This looks like it will leak memory.

[Ilya Sherman, 2012/08/23 07:50:54]

nit: Does this need to be a CHECK, or would a DCHECK suffice?  (Ditto below.)

[kaiwang, 2012/08/24 04:17:58]

You mean iterator? It's in a scoped_ptr

[Ilya Sherman, 2012/08/29 08:48:16]

Sorry, you're right, this should be fine.

+}
+
+bool HistogramSamples::Add(PickleIterator* iter) {
+  int64 sum;
+  HistogramBase::Count redundant_count;
+
+  if (!iter->ReadInt64(&sum) || !iter->ReadInt(&redundant_count))
+    return false;
+  sum_ += sum;
+  redundant_count_ += redundant_count;
+
+  scoped_ptr<SampleCountIterator> pickle_iter(
+      new SampleCountPickleIterator(iter));

[Ilya Sherman, 2012/08/23 07:50:54]

nit: Why not stack-allocate this?

[kaiwang, 2012/08/24 04:17:58]

Done.

+  return AddSubtractHelper(pickle_iter.get(), true);
+}
+
+void HistogramSamples::Subtract(const HistogramSamples& other) {
+  sum_ -= other.sum();
+  redundant_count_ -= other.redundant_count();
+  CHECK(AddSubtractHelper(other.Iterator().get(), false));
+}
+
+bool HistogramSamples::Serialize(Pickle* pickle) const {
+  if (!pickle->WriteInt64(sum_) || !pickle->WriteInt(redundant_count_))
+    return false;
+
+  HistogramBase::Sample min;
+  HistogramBase::Sample max;
+  HistogramBase::Count count;
+  for (scoped_ptr<SampleCountIterator> it = Iterator();
+       !it->Done();
+       it->Next()) {
+    it->Get(&min, &max, &count);
+    if (!pickle->WriteInt(min) ||
+        !pickle->WriteInt(max) ||
+        !pickle->WriteInt(count))
+      return false;
+  }
+  return true;
+}
+
+SampleCountIterator::~SampleCountIterator() {}
+
+}  // namespace base