C++ Readability Review for caitkp

C++ Readability Review for caitkp

BUG=10927483
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227832

[milos, 2013-10-01 17:54:21]

This changelist looks pretty good - thanks for following the style guide
closely. Since a readability review is very detailed, I've have a few comments
and recommendations. I did not mark all cases where those comments apply. Please
go over the whole chaneglist and fix everywhere. After that we should be to
finish the readability review.

Thanks,

Milos

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h
File base/callback_registry.h (right):

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:48: // };
Possibly add DISALLOW_COPY_AND_ASSIGN() in the example?

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:84: iter_(iter) {}
A nit - {} can be used when the whole expression fits to a single line. Here the
constructor spans 3 lines and thus } should be on the next line.

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:88: (*iter_).Reset();
FYI, or iter_->Reset();

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:149: : active_iterator_count_(0) {}
This could fit on the previous line, see
https://www.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml#Vertical_Wh...

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:158: Iterator GetIterator() {
Could the function be declared const?

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:187: : public
internal::CallbackRegistryBase<Callback<void(void)> > {
FYI, with C++11 there is no need for a space between > >

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:193: void Notify() {
Could this function be const?

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry_uni...
File base/callback_registry_unittest.cc (right):

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry_uni...
base/callback_registry_unittest.cc:23: int total_;
In general all data members should be private, see
https://www.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml#Access_Control

[Cait (Slow), 2013-10-01 22:51:27]

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h
File base/callback_registry.h (right):

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:48: // };
On 2013/10/01 17:54:21, milos wrote:
> Possibly add DISALLOW_COPY_AND_ASSIGN() in the example?

Done.

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:84: iter_(iter) {}
On 2013/10/01 17:54:21, milos wrote:
> A nit - {} can be used when the whole expression fits to a single line. Here
the
> constructor spans 3 lines and thus } should be on the next line.

Done.

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:88: (*iter_).Reset();
On 2013/10/01 17:54:21, milos wrote:
> FYI, or iter_->Reset();

Done.

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:149: : active_iterator_count_(0) {}
On 2013/10/01 17:54:21, milos wrote:
> This could fit on the previous line, see
>
https://www.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml#Vertical_Wh...

Done.

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:158: Iterator GetIterator() {
On 2013/10/01 17:54:21, milos wrote:
> Could the function be declared const?

When the Iterator is created, it increments active_iterator_count_ on the
callbackList object, so it can't be declared const.

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:187: : public
internal::CallbackRegistryBase<Callback<void(void)> > {
On 2013/10/01 17:54:21, milos wrote:
> FYI, with C++11 there is no need for a space between > >

AFAIK, Chrome doesn't support C++11 yet, so the space is still needed here.

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry.h#n...
base/callback_registry.h:193: void Notify() {
On 2013/10/01 17:54:21, milos wrote:
> Could this function be const?

This calls GetIterator, so can't be const for the same reason as above.

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry_uni...
File base/callback_registry_unittest.cc (right):

https://codereview.chromium.org/23645019/diff/5001/base/callback_registry_uni...
base/callback_registry_unittest.cc:23: int total_;
On 2013/10/01 17:54:21, milos wrote:
> In general all data members should be private, see
>
https://www.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml#Access_Control

Done.

[milos, 2013-10-03 15:49:43]

Hi Cait,

I think we're done with the readability review. I have a last set of comments
and once they are fixed we can close this review.

Thank you,

Milos

https://codereview.chromium.org/23645019/diff/10001/base/callback_list.h
File base/callback_list.h (right):

https://codereview.chromium.org/23645019/diff/10001/base/callback_list.h#newc...
base/callback_list.h:213: Callback<void(A1)> > {
This would fit on the previous line, so it would be preferred.

https://codereview.chromium.org/23645019/diff/10001/base/callback_list.h#newc...
base/callback_list.h:223: while((cb = it.GetNext()) != NULL) {
Missing a space between "while" and "("

https://codereview.chromium.org/23645019/diff/10001/base/callback_list.h#newc...
base/callback_list.h:235: Callback<void(A1, A2)> > {
Ditto

https://codereview.chromium.org/23645019/diff/10001/base/callback_list.h#newc...
base/callback_list.h:246: while((cb = it.GetNext()) != NULL) {
Ditto

https://codereview.chromium.org/23645019/diff/10001/base/callback_list_unitte...
File base/callback_list_unittest.cc (right):

https://codereview.chromium.org/23645019/diff/10001/base/callback_list_unitte...
base/callback_list_unittest.cc:23: int total() { return total_; }
A nit - the function could be declared const, see
https://www.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml#Use_of_const

https://codereview.chromium.org/23645019/diff/10001/base/callback_list_unitte...
base/callback_list_unittest.cc:56: cb_reg_(cb_reg) {}
} should be on the next line.

[Cait (Slow), 2013-10-03 15:58:11]

Thanks!

https://codereview.chromium.org/23645019/diff/10001/base/callback_list.h
File base/callback_list.h (right):

https://codereview.chromium.org/23645019/diff/10001/base/callback_list.h#newc...
base/callback_list.h:213: Callback<void(A1)> > {
On 2013/10/03 15:49:43, milos wrote:
> This would fit on the previous line, so it would be preferred.

Done.

https://codereview.chromium.org/23645019/diff/10001/base/callback_list.h#newc...
base/callback_list.h:223: while((cb = it.GetNext()) != NULL) {
On 2013/10/03 15:49:43, milos wrote:
> Missing a space between "while" and "("

Done.

https://codereview.chromium.org/23645019/diff/10001/base/callback_list.h#newc...
base/callback_list.h:235: Callback<void(A1, A2)> > {
On 2013/10/03 15:49:43, milos wrote:
> Ditto

Done.

https://codereview.chromium.org/23645019/diff/10001/base/callback_list.h#newc...
base/callback_list.h:246: while((cb = it.GetNext()) != NULL) {
On 2013/10/03 15:49:43, milos wrote:
> Ditto

Done.

https://codereview.chromium.org/23645019/diff/10001/base/callback_list_unitte...
File base/callback_list_unittest.cc (right):

https://codereview.chromium.org/23645019/diff/10001/base/callback_list_unitte...
base/callback_list_unittest.cc:23: int total() { return total_; }
On 2013/10/03 15:49:43, milos wrote:
> A nit - the function could be declared const, see
> https://www.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml#Use_of_const

Done.

https://codereview.chromium.org/23645019/diff/10001/base/callback_list_unitte...
base/callback_list_unittest.cc:56: cb_reg_(cb_reg) {}
On 2013/10/03 15:49:43, milos wrote:
> } should be on the next line.

Done.

[milos, 2013-10-03 16:39:39]

lgtm

Thank you Cait. Once the changelist gets submitted, C++ readability will be
granted to you automatically.

Milos

[Cait (Slow), 2013-10-03 16:46:01]

Albert: PTAL. This is mostly just nits and clean-up (from the readability
review).

Thanks!

[Cait (Slow), 2013-10-09 15:21:51]

ajwong: ping :) -- Thanks!

[awong, 2013-10-09 15:23:18]

LGTM

[commit-bot: I haz the power, 2013-10-09 15:33:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/23645019/18001

[commit-bot: I haz the power, 2013-10-09 23:28:24]

Change committed as 227832