SampleSet -> HistogramSamples (will be reused by SparseHistogram)

base/metrics/histogram_samples.h

+// 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.
+
+// HistogramSamples is a container storing all samples of a histogram.

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

Optional nit: Perhaps move this comment closer to the class declaration?

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

Done.

+
+#ifndef BASE_METRICS_HISTOGRAM_SAMPLES_H_
+#define BASE_METRICS_HISTOGRAM_SAMPLES_H_
+
+#include <utility>

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

nit: What is this used for?

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

Done.

+
+#include "base/basictypes.h"
+#include "base/metrics/histogram_base.h"
+#include "base/memory/scoped_ptr.h"
+
+class Pickle;
+class PickleIterator;
+
+namespace base {
+
+class SampleCountIterator;
+
+class BASE_EXPORT HistogramSamples {
+ public:
+  HistogramSamples();
+  virtual ~HistogramSamples();
+
+  virtual void Accumulate(HistogramBase::Sample value,
+                          HistogramBase::Count count) = 0;
+  virtual HistogramBase::Count GetCount(HistogramBase::Sample value) const = 0;
+  virtual HistogramBase::Count TotalCount() const = 0;
+
+  virtual void Add(const HistogramSamples& other);
+  virtual bool Add(PickleIterator* iter);  // Add from serialized samples.

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

Why is this method needed, rather than first reading a HistogramSamples from a
Pickle, and then using the regular Add method?  Function overloading is
generally discouraged in Chromium (though it looks like the style guide has been
updated to allow it in some cases).

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

Changed to "AddFromPickle"

+  virtual void Subtract(const HistogramSamples& other);
+
+  virtual scoped_ptr<SampleCountIterator> Iterator() const = 0;

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

Optional nit: Perhaps CreateIterator()?

+  virtual bool Serialize(Pickle* pickle) const;
+
+  // Accessor fuctions.
+  int64 sum() const { return sum_; }
+  HistogramBase::Count redundant_count() const { return redundant_count_; }
+
+ protected:
+  // Add/subtract sample counts data from the iterator.
+  virtual bool AddSubtractHelper(SampleCountIterator* iter, bool is_add) = 0;

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

nit: Please give this a more descriptive name.  I cannot tell what this method
is meant to do, but it's also pure-virtual, so I really have no good way to find
out.

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

nit: Interfaces like this should take an enumerated value rather than a boolean
as a parameter, so that the meaning of the parameter is clear from the call
site.

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

Changed to AddSubtractImpl

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

(bump)

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

If it's not add, then it's subtract, added comment about this. A lot of
functions are using bool in this way.
Changing this bool to a enum complicates the implementation (switch ...). Also
the reader of the function signature will be harder to get the idea.

+
+  int64 sum_;

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

nit: Members should be private:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Access_Control
(Ditto below)

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

Done.

+
+  // |redundant_count_| helps identify memory corruption, we reduntantly save
+  // the number of samples we've accumulated. We can compare this count to the

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

nit: Split the first sentence as:

|redundant_count| helps identify memory corruption.  It redundantly stores the
total number of samples accumulated in the histogram.

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

Done.

+  // sum of the counts (TotalCount() function), and detect problems.
+  // Note, depending on the implementation of different histogram types, there
+  // might be races during histogram accumulation and snapshotting that we
+  // choose to accept. In this case, the tallies might mismatch even when no
+  // memory corruption has happened.
+  HistogramBase::Count redundant_count_;
+};
+
+class BASE_EXPORT SampleCountIterator {
+ public:
+  virtual ~SampleCountIterator();
+
+  virtual bool Done() = 0;

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

nit: Perhaps IsDone() or IsAtEnd()?

+  virtual void Next() = 0;

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

nit: Perhaps Advance()?

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

Done.

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

These 2 names are very conventional in google code:

https://cs.corp.google.com/#search/&sq=package:google3%20-file:%5Eexperimenta...

+  virtual void Get(HistogramBase::Sample* min,
+                   HistogramBase::Sample* max,
+                   HistogramBase::Count* count) = 0;
+};
+
+}  // namespace base
+
+#endif  // BASE_METRICS_HISTOGRAM_SAMPLES_H_