C++ Readability Review for caitkp

base/callback_registry.h

 // This file was GENERATED by command:
 //     pump.py callback_registry.h.pump
 // DO NOT EDIT BY HAND!!!
 
 
 // Copyright 2013 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.
 
 #ifndef BASE_CALLBACK_REGISTRY_H_
@@ skipping 27 matching lines @@
 //   RegisterCallback(const OnFooCallback& cb) {
 //     return callback_registry_.Add(cb);
 //   }
 //
 //  private:
 //   void NotifyFoo(const Foo& foo) {
 //      callback_registry_.Notify(foo);
 //   }
 //
 //   base::CallbackRegistry<void(const Foo&)> callback_registry_;
 // };

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

Possibly add DISALLOW_COPY_AND_ASSIGN() in the example?

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

Done.

 //
 //
 // class MyWidgetListener {
 //  public:
 //   MyWidgetListener::MyWidgetListener() {
 //     foo_subscription_ = MyWidget::GetCurrent()->RegisterCallback(
 //             base::Bind(&MyWidgetListener::OnFoo, this)));
 //   }
 //
 //   MyWidgetListener::~MyWidgetListener() {
@@ skipping 15 matching lines @@
 namespace internal {
 
 template <typename CallbackType>
 class CallbackRegistryBase {
  public:
   class Subscription {
    public:
     Subscription(CallbackRegistryBase<CallbackType>* list,
                  typename std::list<CallbackType>::iterator iter)
         : list_(list),
           iter_(iter) {}

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

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.

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

Done.

 
     ~Subscription() {
       if (list_->active_iterator_count_)
         (*iter_).Reset();

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

FYI, or iter_->Reset();

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

Done.

       else
         list_->callbacks_.erase(iter_);
     }
 
    private:
     CallbackRegistryBase<CallbackType>* list_;
     typename std::list<CallbackType>::iterator iter_;
 
     DISALLOW_COPY_AND_ASSIGN(Subscription);
   };
@@ skipping 40 matching lines @@
       }
       return cb;
     }
 
    private:
     CallbackRegistryBase<CallbackType>* list_;
     typename std::list<CallbackType>::iterator list_iter_;
   };
 
   CallbackRegistryBase()
       : active_iterator_count_(0) {}

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

This could fit on the previous line, see
https://www.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml#Vertical_Wh...

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

Done.

 
   ~CallbackRegistryBase() {
     DCHECK_EQ(0, active_iterator_count_);
     DCHECK_EQ(0U, callbacks_.size());
   }
 
   // Returns an instance of a CallbackRegistryBase::Iterator which can be used
   // to run callbacks.
   Iterator GetIterator() {

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

Could the function be declared const?

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

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

     return Iterator(this);
   }
 
   // Compact the list: remove any entries which were NULLed out during
   // iteration.
   void Compact() {
     typename std::list<CallbackType>::iterator it = callbacks_.begin();
     while (it != callbacks_.end()) {
       if ((*it).is_null())
         it = callbacks_.erase(it);
       else
         ++it;
     }
   }
 
  private:
   std::list<CallbackType> callbacks_;
   int active_iterator_count_;
 
   DISALLOW_COPY_AND_ASSIGN(CallbackRegistryBase);
 };
 
 }  // namespace internal
 
 template <typename Sig> class CallbackRegistry;
 
 template <>
 class CallbackRegistry<void(void)>
     : public internal::CallbackRegistryBase<Callback<void(void)> > {

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

FYI, with C++11 there is no need for a space between > >

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

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

  public:
   typedef Callback<void(void)> CallbackType;
 
   CallbackRegistry() {}
 
   void Notify() {

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

Could this function be const?

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

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

     internal::CallbackRegistryBase<CallbackType>::Iterator it =
         this->GetIterator();
     CallbackType* cb;
     while((cb = it.GetNext()) != NULL) {
       cb->Run();
     }
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(CallbackRegistry);
@@ skipping 172 matching lines @@
     }
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(CallbackRegistry);
 };
 
 }  // namespace base
 
 #endif  // BASE_CALLBACK_REGISTRY_H
+// NOSUBMIT: Force git to add file for readability.