Readability review.
Review URL: http://codereview.chromium.org/2729018
diff --git a/metrics/counter.cc b/metrics/counter.cc
index 58dbae4..b81a0d1 100644
--- a/metrics/counter.cc
+++ b/metrics/counter.cc
@@ -4,7 +4,7 @@
#include "counter.h"
-#include <sys/file.h>
+#include <fcntl.h>
#include <base/eintr_wrapper.h>
#include <base/logging.h>
@@ -12,26 +12,28 @@
namespace chromeos_metrics {
// TaggedCounter::Record implementation.
-void TaggedCounter::Record::Init(int tag, int count) {
+void TaggedCounter::Record::Init(int32 tag, int32 count) {
tag_ = tag;
count_ = (count > 0) ? count : 0;
}
-void TaggedCounter::Record::Add(int count) {
+void TaggedCounter::Record::Add(int32 count) {
if (count <= 0)
return;
- count_ += count;
-
- // Saturates on postive overflow.
- if (count_ < 0) {
- count_ = INT_MAX;
- }
+ // Saturates on positive overflow.
+ int64 new_count = static_cast<int64>(count_) + static_cast<int64>(count);
+ if (new_count > kint32max)
+ count_ = kint32max;
+ else
+ count_ = static_cast<int32>(new_count);
}
// TaggedCounter implementation.
TaggedCounter::TaggedCounter()
- : filename_(NULL), reporter_(NULL), reporter_handle_(NULL),
+ : filename_(NULL),
+ reporter_(NULL),
+ reporter_handle_(NULL),
record_state_(kRecordInvalid) {}
TaggedCounter::~TaggedCounter() {}
@@ -45,7 +47,7 @@
record_state_ = kRecordInvalid;
}
-void TaggedCounter::Update(int tag, int count) {
+void TaggedCounter::Update(int32 tag, int32 count) {
UpdateInternal(tag,
count,
false); // No flush.
@@ -57,7 +59,7 @@
true); // Do flush.
}
-void TaggedCounter::UpdateInternal(int tag, int count, bool flush) {
+void TaggedCounter::UpdateInternal(int32 tag, int32 count, bool flush) {
if (flush) {
// Flushing but record is null, so nothing to do.
if (record_state_ == kRecordNull)
@@ -104,11 +106,10 @@
record_state_ = kRecordNullDirty;
return;
}
-
record_state_ = kRecordNull;
}
-void TaggedCounter::ReportRecord(int tag, bool flush) {
+void TaggedCounter::ReportRecord(int32 tag, bool flush) {
// If no valid record, there's nothing to report.
if (record_state_ != kRecordValid) {
DCHECK_EQ(record_state_, kRecordNull);
@@ -126,7 +127,7 @@
record_state_ = kRecordNullDirty;
}
-void TaggedCounter::UpdateRecord(int tag, int count, bool flush) {
+void TaggedCounter::UpdateRecord(int32 tag, int32 count, bool flush) {
if (flush) {
DCHECK(record_state_ == kRecordNull || record_state_ == kRecordNullDirty);
return;
diff --git a/metrics/counter.h b/metrics/counter.h
index 876b107..1cfcb51 100644
--- a/metrics/counter.h
+++ b/metrics/counter.h
@@ -5,6 +5,7 @@
#ifndef METRICS_COUNTER_H_
#define METRICS_COUNTER_H_
+#include <base/basictypes.h>
#include <gtest/gtest_prod.h> // for FRIEND_TEST
namespace chromeos_metrics {
@@ -18,6 +19,11 @@
// event counts. The aggregated count is reported through the
// callback when the counter is explicitly flushed or when data for a
// new tag arrives.
+//
+// The primary reason for using an interface is to allow easier unit
+// testing in clients through mocking thus avoiding file access and
+// callbacks. Of course, it also enables alternative implementations
+// of the counter with additional features.
class TaggedCounterInterface {
public:
// Callback type used for reporting aggregated or flushed data.
@@ -27,7 +33,7 @@
// |handle| is the |reporter_handle| pointer passed through Init.
// |tag| is the tag associated with the aggregated count.
// |count| is aggregated count.
- typedef void (*Reporter)(void* handle, int tag, int count);
+ typedef void (*Reporter)(void* handle, int32 tag, int32 count);
virtual ~TaggedCounterInterface() {}
@@ -44,7 +50,7 @@
// Adds |count| of events for the given |tag|. If there's an
// existing aggregated count for a different tag, it's reported
// through the reporter callback and discarded.
- virtual void Update(int tag, int count) = 0;
+ virtual void Update(int32 tag, int32 count) = 0;
// Reports the current aggregated count (if any) through the
// reporter callback and discards it.
@@ -58,7 +64,7 @@
// Implementation of interface methods.
void Init(const char* filename, Reporter reporter, void* reporter_handle);
- void Update(int tag, int count);
+ void Update(int32 tag, int32 count);
void Flush();
private:
@@ -89,25 +95,25 @@
// Initializes with |tag| and |count|. If |count| is negative,
// |count_| is set to 0.
- void Init(int tag, int count);
+ void Init(int32 tag, int32 count);
// Adds |count| to the current |count_|. Negative |count| is
// ignored. In case of positive overflow, |count_| is saturated to
- // INT_MAX.
- void Add(int count);
+ // kint32max.
+ void Add(int32 count);
- int tag() const { return tag_; }
- int count() const { return count_; }
+ int32 tag() const { return tag_; }
+ int32 count() const { return count_; }
private:
- int tag_;
- int count_;
+ int32 tag_;
+ int32 count_;
};
// Implementation of the Update and Flush methods. Goes through the
// necessary steps to read, report, update, and sync the aggregated
// record.
- void UpdateInternal(int tag, int count, bool flush);
+ void UpdateInternal(int32 tag, int32 count, bool flush);
// If the current cached record is invalid, reads it from persistent
// storage specified through file descriptor |fd| and updates the
@@ -119,13 +125,13 @@
// or the new |tag| is different than the old one, reports the
// aggregated data through the reporter callback and resets the
// cached record.
- void ReportRecord(int tag, bool flush);
+ void ReportRecord(int32 tag, bool flush);
// Updates the cached record given the new |tag| and |count|. This
// method expects either a null cached record, or a valid cached
// record with the same tag as |tag|. If |flush| is true, the method
// asserts that the cached record is null and returns.
- void UpdateRecord(int tag, int count, bool flush);
+ void UpdateRecord(int32 tag, int32 count, bool flush);
// If the cached record state is dirty, updates the persistent
// storage specified through file descriptor |fd| and switches the
diff --git a/metrics/counter_mock.h b/metrics/counter_mock.h
index 22327ac..baa97b0 100644
--- a/metrics/counter_mock.h
+++ b/metrics/counter_mock.h
@@ -24,3 +24,4 @@
} // namespace chromeos_metrics
#endif // METRICS_COUNTER_MOCK_H_
+
diff --git a/metrics/counter_test.cc b/metrics/counter_test.cc
index 605e859..fd5a389 100644
--- a/metrics/counter_test.cc
+++ b/metrics/counter_test.cc
@@ -62,8 +62,8 @@
// Asserts that the record file contains the specified contents.
testing::AssertionResult AssertRecord(const char* expr_tag,
const char* expr_count,
- int expected_tag,
- int expected_count) {
+ int32 expected_tag,
+ int32 expected_count) {
int fd = HANDLE_EINTR(open(kTestRecordFile, O_RDONLY));
if (fd < 0) {
testing::Message msg;
@@ -105,7 +105,7 @@
// Adds a reporter call expectation that the specified tag/count
// callback will be generated.
- void ExpectReporterCall(int tag, int count) {
+ void ExpectReporterCall(int32 tag, int32 count) {
EXPECT_CALL(reporter_, Call(_, tag, count))
.Times(1)
.RetiresOnSaturation();
@@ -113,7 +113,7 @@
// The reporter callback forwards the call to the reporter mock so
// that we can set call expectations.
- static void Reporter(void* handle, int tag, int count) {
+ static void Reporter(void* handle, int32 tag, int32 count) {
TaggedCounterTest* test = static_cast<TaggedCounterTest*>(handle);
ASSERT_FALSE(NULL == test);
test->reporter_.Call(handle, tag, count);
@@ -130,7 +130,7 @@
}
// Returns true if the counter log contains |pattern|, false otherwise.
- bool LogContains(const std::string& pattern) {
+ bool LogContains(const std::string& pattern) const {
return log_.find(pattern) != std::string::npos;
}
@@ -141,7 +141,8 @@
std::string log_;
// Reporter mock to set callback expectations on.
- StrictMock<MockFunction<void(void* handle, int tag, int count)> > reporter_;
+ StrictMock<MockFunction<void(void* handle,
+ int32 tag, int32 count)> > reporter_;
// Pointer to the current test fixture.
static TaggedCounterTest* test_;
@@ -173,11 +174,11 @@
record_.Add(/* count */ -2);
EXPECT_EQ(15, record_.count());
- record_.Add(/* count */ INT_MAX);
- EXPECT_EQ(INT_MAX, record_.count());
+ record_.Add(/* count */ kint32max);
+ EXPECT_EQ(kint32max, record_.count());
record_.Add(/* count */ 1);
- EXPECT_EQ(INT_MAX, record_.count());
+ EXPECT_EQ(kint32max, record_.count());
}
TEST_F(TaggedCounterTest, BadFileLocation) {
