Readability review.

Review URL: http://codereview.chromium.org/2729018
This commit is contained in:
Darin Petkov 2010-06-24 10:13:54 -07:00
parent c2bf95fd8b
commit cd8c317453
4 changed files with 46 additions and 37 deletions

View File

@ -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 @@ void TaggedCounter::Init(const char* filename,
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 @@ void TaggedCounter::Flush() {
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 @@ void TaggedCounter::ReadRecord(int fd) {
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 @@ void TaggedCounter::ReportRecord(int tag, bool flush) {
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;

View File

@ -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 @@ namespace chromeos_metrics {
// 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 @@ class TaggedCounterInterface {
// |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 @@ class TaggedCounterInterface {
// 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 @@ class TaggedCounter : public TaggedCounterInterface {
// 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 @@ class TaggedCounter : public TaggedCounterInterface {
// 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 @@ class TaggedCounter : public TaggedCounterInterface {
// 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

View File

@ -24,3 +24,4 @@ class TaggedCounterMock : public TaggedCounterInterface {
} // namespace chromeos_metrics
#endif // METRICS_COUNTER_MOCK_H_

View File

@ -62,8 +62,8 @@ class TaggedCounterTest : public testing::Test {
// 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 @@ class TaggedCounterTest : public testing::Test {
// 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 @@ class TaggedCounterTest : public testing::Test {
// 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 @@ class TaggedCounterTest : public testing::Test {
}
// 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 @@ class TaggedCounterTest : public testing::Test {
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 @@ TEST_F(RecordTest, Add) {
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) {