Readability review.

Readability review.

[jyrki, 2010-06-16 12:25:54]

Perhaps you could justify the existence of the abstract interface in a comment,
too.

http://codereview.chromium.org/2729018/diff/1/2
File counter.cc (right):

http://codereview.chromium.org/2729018/diff/1/2#newcode26
counter.cc:26: // Saturates on postive overflow.
typo

http://codereview.chromium.org/2729018/diff/1/2#newcode27
counter.cc:27: if (count_ < 0) {
you should not assume anything about positive overflow of signed integers.

consider changing count_ to unsigned and check for the half way

(do you have a test that tests this?)

http://codereview.chromium.org/2729018/diff/1/2#newcode34
counter.cc:34: : filename_(NULL), reporter_(NULL), reporter_handle_(NULL),
examples in the style guide show one member initialized per line

http://codereview.chromium.org/2729018/diff/1/2#newcode73
counter.cc:73: int fd = HANDLE_EINTR(open(filename_, O_RDWR | O_CREAT, S_IRUSR |
S_IWUSR));
if this open(2)

why no #include <fcntl.h>

[petkov, 2010-06-16 17:30:27]

Thank you for the prompt review. I've added a comment motivating the use of
interface and addressed all feedback.

Please take a look.

http://codereview.chromium.org/2729018/diff/1/2
File counter.cc (right):

http://codereview.chromium.org/2729018/diff/1/2#newcode26
counter.cc:26: // Saturates on postive overflow.
On 2010/06/16 12:25:54, jyrki wrote:
> typo

Done.

http://codereview.chromium.org/2729018/diff/1/2#newcode27
counter.cc:27: if (count_ < 0) {
On 2010/06/16 12:25:54, jyrki wrote:
> you should not assume anything about positive overflow of signed integers.
> 
> consider changing count_ to unsigned and check for the half way

I've made all tag/count instances int32 now and I've made the check for overflow
explicit by accumulating into int64 first.

> 
> (do you have a test that tests this?)

Yes, a couple. Look for kint32max (used to be INT_MAX) in counter_test.cc (lines
177 and 180).

http://codereview.chromium.org/2729018/diff/1/2#newcode34
counter.cc:34: : filename_(NULL), reporter_(NULL), reporter_handle_(NULL),
On 2010/06/16 12:25:54, jyrki wrote:
> examples in the style guide show one member initialized per line

Done. I saw the examples but the description wasn't explicit about this, and
I've seen it done both ways in Chromium.

http://codereview.chromium.org/2729018/diff/1/2#newcode73
counter.cc:73: int fd = HANDLE_EINTR(open(filename_, O_RDWR | O_CREAT, S_IRUSR |
S_IWUSR));
On 2010/06/16 12:25:54, jyrki wrote:
> if this open(2)
> 
> why no #include <fcntl.h>

Good point -- no need for flock (yet). Switched to fcntl.h.

[jyrki, 2010-06-16 18:31:06]

some more findings to discuss or fix

http://codereview.chromium.org/2729018/diff/5001/6001
File counter.cc (right):

http://codereview.chromium.org/2729018/diff/5001/6001#newcode104
counter.cc:104: 
be a bit more sparing with empty lines, vertical space is precious

http://codereview.chromium.org/2729018/diff/5001/6002
File counter.h (right):

http://codereview.chromium.org/2729018/diff/5001/6002#newcode8
counter.h:8: #include <base/basictypes.h>
could you use base/integral_types.h

http://codereview.chromium.org/2729018/diff/5001/6004
File counter_test.cc (right):

http://codereview.chromium.org/2729018/diff/5001/6004#newcode65
counter_test.cc:65: int32 expected_tag,
probably a good idea to include base/integral_types.h here too

http://codereview.chromium.org/2729018/diff/5001/6004#newcode133
counter_test.cc:133: bool LogContains(const std::string& pattern) {
could be a const method

http://codereview.chromium.org/2729018/diff/5001/6004#newcode260
counter_test.cc:260: int main(int argc, char** argv) {
in google3 code the normal approach is to link in gunit_main lib, that has a
suitable main function, but perhaps this is ok in chromium

[petkov, 2010-06-16 19:38:03]

Please take another look. Thanks.

http://codereview.chromium.org/2729018/diff/5001/6001
File counter.cc (right):

http://codereview.chromium.org/2729018/diff/5001/6001#newcode104
counter.cc:104: 
On 2010/06/16 18:31:06, jyrki wrote:
> be a bit more sparing with empty lines, vertical space is precious

Done. It's a somewhat subjective trade off between vertical space and
readability.

http://codereview.chromium.org/2729018/diff/5001/6002
File counter.h (right):

http://codereview.chromium.org/2729018/diff/5001/6002#newcode8
counter.h:8: #include <base/basictypes.h>
On 2010/06/16 18:31:06, jyrki wrote:
> could you use base/integral_types.h

Not available in Chromium / Chromium OS. See:

http://src.chromium.org/viewvc/chrome/trunk/src/base/?dir_pagestart=100

We seem to use base/basictypes.h everywhere.

http://codereview.chromium.org/2729018/diff/5001/6004
File counter_test.cc (right):

http://codereview.chromium.org/2729018/diff/5001/6004#newcode65
counter_test.cc:65: int32 expected_tag,
On 2010/06/16 18:31:06, jyrki wrote:
> probably a good idea to include base/integral_types.h here too

Do you mean that I should include base/basictypes.h in this file too although
it's already included by counter.h (which specifies the tag/count types)?

http://codereview.chromium.org/2729018/diff/5001/6004#newcode133
counter_test.cc:133: bool LogContains(const std::string& pattern) {
On 2010/06/16 18:31:06, jyrki wrote:
> could be a const method

Done.

http://codereview.chromium.org/2729018/diff/5001/6004#newcode260
counter_test.cc:260: int main(int argc, char** argv) {
On 2010/06/16 18:31:06, jyrki wrote:
> in google3 code the normal approach is to link in gunit_main lib, that has a
> suitable main function, but perhaps this is ok in chromium

We don't have such a common library in Chromium OS. Maybe it's a good idea to
add one at some point.

[jyrki, 2010-06-23 10:36:42]

LGTM