[Utility] Small improvements to the Broadcaster class (NFC)

I touched the Broadcaster class earlier today (r361544) and noticed a
few things that could be improved. This patch includes variety of small
fixes: use early returns, use LLDB_LOG macro, use doxygen comments and
finally format the class.

llvm-svn: 361597

lldb/include/lldb/Utility/Broadcaster.h

@@ -29,14 +29,14 @@ class Broadcaster;
 class EventData;
 class Listener;
 class Stream;
-}
+} // namespace lldb_private
 
 namespace lldb_private {
 
-// lldb::BroadcastEventSpec
+/// lldb::BroadcastEventSpec
-//
+///
-// This class is used to specify a kind of event to register for.  The Debugger
+/// This class is used to specify a kind of event to register for.  The
-// maintains a list of BroadcastEventSpec's and when it is made
+/// Debugger maintains a list of BroadcastEventSpec's and when it is made
 class BroadcastEventSpec {
 public:
   BroadcastEventSpec(ConstString broadcaster_class, uint32_t event_bits)
@@ -48,19 +48,19 @@ public:
 
   uint32_t GetEventBits() const { return m_event_bits; }
 
-  // Tell whether this BroadcastEventSpec is contained in in_spec. That is: (a)
+  /// Tell whether this BroadcastEventSpec is contained in in_spec. That is:
-  // the two spec's share the same broadcaster class (b) the event bits of this
+  /// (a) the two spec's share the same broadcaster class (b) the event bits of
-  // spec are wholly contained in those of in_spec.
+  /// this spec are wholly contained in those of in_spec.
   bool IsContainedIn(const BroadcastEventSpec &in_spec) const {
     if (m_broadcaster_class != in_spec.GetBroadcasterClass())
       return false;
     uint32_t in_bits = in_spec.GetEventBits();
     if (in_bits == m_event_bits)
       return true;
-    else {
+
-      if ((m_event_bits & in_bits) != 0 && (m_event_bits & ~in_bits) == 0)
+    if ((m_event_bits & in_bits) != 0 && (m_event_bits & ~in_bits) == 0)
-        return true;
+      return true;
-    }
+
     return false;
   }
 
@@ -81,10 +81,9 @@ protected:
   BroadcasterManager();
 
 public:
-  // Listeners hold onto weak pointers to their broadcaster managers.  So they
+  /// Listeners hold onto weak pointers to their broadcaster managers.  So they
-  // must be made into shared pointers, which you do with
+  /// must be made into shared pointers, which you do with
-  // MakeBroadcasterManager.
+  /// MakeBroadcasterManager.
-
   static lldb::BroadcasterManagerSP MakeBroadcasterManager();
 
   ~BroadcasterManager() = default;
@@ -179,8 +178,8 @@ private:
     bool operator()(const event_listener_key &input) const {
       if (input.second == m_listener_sp)
         return true;
-      else
+
-        return false;
+      return false;
     }
 
   private:
@@ -197,15 +196,15 @@ private:
     bool operator()(const event_listener_key &input) const {
       if (input.second.get() == m_listener)
         return true;
-      else
+
-        return false;
+      return false;
     }
 
     bool operator()(const lldb::ListenerSP &input) const {
       if (input.get() == m_listener)
         return true;
-      else
+
-        return false;
+      return false;
     }
 
   private:
@@ -413,32 +412,30 @@ public:
   }
 
   /// Restore the state of the Broadcaster from a previous hijack attempt.
-  ///
   void RestoreBroadcaster() { m_broadcaster_sp->RestoreBroadcaster(); }
 
-  // This needs to be filled in if you are going to register the broadcaster
+  /// This needs to be filled in if you are going to register the broadcaster
-  // with the broadcaster manager and do broadcaster class matching.
+  /// with the broadcaster manager and do broadcaster class matching.
-  // FIXME: Probably should make a ManagedBroadcaster subclass with all the bits
+  /// FIXME: Probably should make a ManagedBroadcaster subclass with all the
-  // needed to work
+  /// bits needed to work with the BroadcasterManager, so that it is clearer
-  // with the BroadcasterManager, so that it is clearer how to add one.
+  /// how to add one.
   virtual ConstString &GetBroadcasterClass() const;
 
   lldb::BroadcasterManagerSP GetManager();
 
 protected:
-  // BroadcasterImpl contains the actual Broadcaster implementation.  The
+  /// BroadcasterImpl contains the actual Broadcaster implementation.  The
-  // Broadcaster makes a BroadcasterImpl which lives as long as it does.  The
+  /// Broadcaster makes a BroadcasterImpl which lives as long as it does.  The
-  // Listeners & the Events hold a weak pointer to the BroadcasterImpl, so that
+  /// Listeners & the Events hold a weak pointer to the BroadcasterImpl, so
-  // they can survive if a Broadcaster they were listening to is destroyed w/o
+  /// that they can survive if a Broadcaster they were listening to is
-  // their being able to unregister from it (which can happen if the
+  /// destroyed w/o their being able to unregister from it (which can happen if
-  // Broadcasters & Listeners are being destroyed on separate threads
+  /// the Broadcasters & Listeners are being destroyed on separate threads
-  // simultaneously. The Broadcaster itself can't be shared out as a weak
+  /// simultaneously. The Broadcaster itself can't be shared out as a weak
-  // pointer, because some things that are broadcasters (e.g. the Target and
+  /// pointer, because some things that are broadcasters (e.g. the Target and
-  // the Process) are shared in their own right.
+  /// the Process) are shared in their own right.
-  //
+  ///
-  // For the most part, the Broadcaster functions dispatch to the
+  /// For the most part, the Broadcaster functions dispatch to the
-  // BroadcasterImpl, and are documented in the public Broadcaster API above.
+  /// BroadcasterImpl, and are documented in the public Broadcaster API above.
-
   class BroadcasterImpl {
     friend class Listener;
     friend class Broadcaster;
@@ -505,7 +502,6 @@ protected:
 
     const char *GetHijackingListenerName();
 
-    //
     typedef llvm::SmallVector<std::pair<lldb::ListenerWP, uint32_t>, 4>
         collection;
     typedef std::map<uint32_t, std::string> event_names_map;
@@ -513,22 +509,28 @@ protected:
     llvm::SmallVector<std::pair<lldb::ListenerSP, uint32_t &>, 4>
     GetListeners();
 
-    Broadcaster &m_broadcaster;    ///< The broadcaster that this implements
+    /// The broadcaster that this implements.
-    event_names_map m_event_names; ///< Optionally define event names for
+    Broadcaster &m_broadcaster;
-                                   ///readability and logging for each event bit
+
-    collection m_listeners; ///< A list of Listener / event_mask pairs that are
+    /// Optionally define event names for readability and logging for each
-                            ///listening to this broadcaster.
+    /// event bit.
-    std::recursive_mutex
+    event_names_map m_event_names;
-        m_listeners_mutex; ///< A mutex that protects \a m_listeners.
+
-    std::vector<lldb::ListenerSP> m_hijacking_listeners; // A simple mechanism
+    /// A list of Listener / event_mask pairs that are listening to this
-                                                         // to intercept events
+    /// broadcaster.
-                                                         // from a broadcaster
+    collection m_listeners;
-    std::vector<uint32_t> m_hijacking_masks; // At some point we may want to
+
-                                             // have a stack or Listener
+    /// A mutex that protects \a m_listeners.
-    // collections, but for now this is just for private hijacking.
+    std::recursive_mutex m_listeners_mutex;
+
+    /// A simple mechanism to intercept events from a broadcaster
+    std::vector<lldb::ListenerSP> m_hijacking_listeners;
+
+    /// At some point we may want to have a stack or Listener collections, but
+    /// for now this is just for private hijacking.
+    std::vector<uint32_t> m_hijacking_masks;
 
   private:
-    // For Broadcaster only
     DISALLOW_COPY_AND_ASSIGN(BroadcasterImpl);
   };
 
@@ -540,14 +542,13 @@ protected:
   const char *GetHijackingListenerName() {
     return m_broadcaster_sp->GetHijackingListenerName();
   }
-  // Classes that inherit from Broadcaster can see and modify these
 
 private:
-  // For Broadcaster only
   BroadcasterImplSP m_broadcaster_sp;
   lldb::BroadcasterManagerSP m_manager_sp;
-  const ConstString
+
-      m_broadcaster_name; ///< The name of this broadcaster object.
+  /// The name of this broadcaster object.
+  const ConstString m_broadcaster_name;
 
   DISALLOW_COPY_AND_ASSIGN(Broadcaster);
 };

lldb/source/Utility/Broadcaster.cpp

@@ -30,9 +30,8 @@ Broadcaster::Broadcaster(BroadcasterManagerSP manager_sp, const char *name)
     : m_broadcaster_sp(std::make_shared<BroadcasterImpl>(*this)),
       m_manager_sp(std::move(manager_sp)), m_broadcaster_name(name) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
-  if (log)
+  LLDB_LOG(log, "{0} Broadcaster::Broadcaster(\"{1}\")",
-    log->Printf("%p Broadcaster::Broadcaster(\"%s\")",
+           static_cast<void *>(this), GetBroadcasterName().AsCString());
-                static_cast<void *>(this), GetBroadcasterName().AsCString());
 }
 
 Broadcaster::BroadcasterImpl::BroadcasterImpl(Broadcaster &broadcaster)
@@ -41,9 +40,8 @@ Broadcaster::BroadcasterImpl::BroadcasterImpl(Broadcaster &broadcaster)
 
 Broadcaster::~Broadcaster() {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
-  if (log)
+  LLDB_LOG(log, "{0} Broadcaster::~Broadcaster(\"{1}\")",
-    log->Printf("%p Broadcaster::~Broadcaster(\"%s\")",
+           static_cast<void *>(this), GetBroadcasterName().AsCString());
-                static_cast<void *>(this), m_broadcaster_name.AsCString());
 
   Clear();
 }
@@ -213,8 +211,7 @@ void Broadcaster::BroadcasterImpl::PrivateBroadcastEvent(EventSP &event_sp,
       hijacking_listener_sp.reset();
   }
 
-  Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_EVENTS));
+  if (Log *log = lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_EVENTS)) {
-  if (log) {
     StreamString event_description;
     event_sp->Dump(&event_description);
     log->Printf("%p Broadcaster(\"%s\")::BroadcastEvent (event_sp = {%s}, "
@@ -225,18 +222,16 @@ void Broadcaster::BroadcasterImpl::PrivateBroadcastEvent(EventSP &event_sp,
   }
 
   if (hijacking_listener_sp) {
-    if (unique &&
+    if (unique && hijacking_listener_sp->PeekAtNextEventForBroadcasterWithType(
-        hijacking_listener_sp->PeekAtNextEventForBroadcasterWithType(
+                      &m_broadcaster, event_type))
-            &m_broadcaster, event_type))
       return;
     hijacking_listener_sp->AddEvent(event_sp);
   } else {
     for (auto &pair : GetListeners()) {
       if (!(pair.second & event_type))
         continue;
-      if (unique &&
+      if (unique && pair.first->PeekAtNextEventForBroadcasterWithType(
-          pair.first->PeekAtNextEventForBroadcasterWithType(&m_broadcaster,
+                        &m_broadcaster, event_type))
-                                                            event_type))
         continue;
 
       pair.first->AddEvent(event_sp);
@@ -267,11 +262,11 @@ bool Broadcaster::BroadcasterImpl::HijackBroadcaster(
   std::lock_guard<std::recursive_mutex> guard(m_listeners_mutex);
 
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_EVENTS));
-  if (log)
+  LLDB_LOG(
-    log->Printf(
+      log,
-        "%p Broadcaster(\"%s\")::HijackBroadcaster (listener(\"%s\")=%p)",
+      "{0} Broadcaster(\"{1}\")::HijackBroadcaster (listener(\"{2}\")={3})",
-        static_cast<void *>(this), GetBroadcasterName(),
+      static_cast<void *>(this), GetBroadcasterName(),
-        listener_sp->m_name.c_str(), static_cast<void *>(listener_sp.get()));
+      listener_sp->m_name.c_str(), static_cast<void *>(listener_sp.get()));
   m_hijacking_listeners.push_back(listener_sp);
   m_hijacking_masks.push_back(event_mask);
   return true;
@@ -288,24 +283,22 @@ bool Broadcaster::BroadcasterImpl::IsHijackedForEvent(uint32_t event_mask) {
 const char *Broadcaster::BroadcasterImpl::GetHijackingListenerName() {
   if (m_hijacking_listeners.size()) {
     return m_hijacking_listeners.back()->GetName();
-  } else {
-    return nullptr;
   }
+  return nullptr;
 }
 
 void Broadcaster::BroadcasterImpl::RestoreBroadcaster() {
   std::lock_guard<std::recursive_mutex> guard(m_listeners_mutex);
 
   if (!m_hijacking_listeners.empty()) {
+    ListenerSP listener_sp = m_hijacking_listeners.back();
     Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_EVENTS));
-    if (log) {
+    LLDB_LOG(log,
-      ListenerSP listener_sp = m_hijacking_listeners.back();
+             "{0} Broadcaster(\"{1}\")::RestoreBroadcaster (about to pop "
-      log->Printf("%p Broadcaster(\"%s\")::RestoreBroadcaster (about to pop "
+             "listener(\"{2}\")={3})",
-                  "listener(\"%s\")=%p)",
+             static_cast<void *>(this), GetBroadcasterName(),
-                  static_cast<void *>(this), GetBroadcasterName(),
+             listener_sp->m_name.c_str(),
-                  listener_sp->m_name.c_str(),
+             static_cast<void *>(listener_sp.get()));
-                  static_cast<void *>(listener_sp.get()));
-    }
     m_hijacking_listeners.pop_back();
   }
   if (!m_hijacking_masks.empty())
@@ -320,9 +313,8 @@ ConstString &Broadcaster::GetBroadcasterClass() const {
 bool BroadcastEventSpec::operator<(const BroadcastEventSpec &rhs) const {
   if (GetBroadcasterClass() == rhs.GetBroadcasterClass()) {
     return GetEventBits() < rhs.GetEventBits();
-  } else {
-    return GetBroadcasterClass() < rhs.GetBroadcasterClass();
   }
+  return GetBroadcasterClass() < rhs.GetBroadcasterClass();
 }
 
 BroadcastEventSpec &BroadcastEventSpec::
@@ -378,17 +370,16 @@ bool BroadcasterManager::UnregisterListenerForEvents(
     iter = find_if(m_event_map.begin(), end_iter, predicate);
     if (iter == end_iter) {
       break;
-    } else {
-      uint32_t iter_event_bits = (*iter).first.GetEventBits();
-      removed_some = true;
-
-      if (event_bits_to_remove != iter_event_bits) {
-        uint32_t new_event_bits = iter_event_bits & ~event_bits_to_remove;
-        to_be_readded.push_back(BroadcastEventSpec(
-            event_spec.GetBroadcasterClass(), new_event_bits));
-      }
-      m_event_map.erase(iter);
     }
+    uint32_t iter_event_bits = (*iter).first.GetEventBits();
+    removed_some = true;
+
+    if (event_bits_to_remove != iter_event_bits) {
+      uint32_t new_event_bits = iter_event_bits & ~event_bits_to_remove;
+      to_be_readded.push_back(
+          BroadcastEventSpec(event_spec.GetBroadcasterClass(), new_event_bits));
+    }
+    m_event_map.erase(iter);
   }
 
   // Okay now add back the bits that weren't completely removed:
@@ -408,8 +399,8 @@ ListenerSP BroadcasterManager::GetListenerForEventSpec(
                  BroadcastEventSpecMatches(event_spec));
   if (iter != end_iter)
     return (*iter).second;
-  else
+
-    return nullptr;
+  return nullptr;
 }
 
 void BroadcasterManager::RemoveListener(Listener *listener) {
@@ -427,8 +418,8 @@ void BroadcasterManager::RemoveListener(Listener *listener) {
     iter = find_if(m_event_map.begin(), end_iter, predicate);
     if (iter == end_iter)
       break;
-    else
+
-      m_event_map.erase(iter);
+    m_event_map.erase(iter);
   }
 }
 
@@ -444,8 +435,8 @@ void BroadcasterManager::RemoveListener(const lldb::ListenerSP &listener_sp) {
     iter = find_if(m_event_map.begin(), end_iter, predicate);
     if (iter == end_iter)
       break;
-    else
+
-      m_event_map.erase(iter);
+    m_event_map.erase(iter);
   }
 }