Add comments and type information to break_point

Non functional changes to make the code more readable.

hphp/runtime/debugger/break_point.cpp

@@ -142,7 +142,7 @@ const char *BreakPointInfo::GetInterruptName(InterruptType interrupt) {
 BreakPointInfo::BreakPointInfo(bool regex, State state,
                                const std::string &file, int line)
     : m_index(0), m_state(state), m_valid(true),
-      m_interrupt(BreakPointReached),
+      m_interruptType(BreakPointReached),
       m_file(file), m_line1(line), m_line2(line),
       m_regex(regex), m_check(false) {
   TRACE(2, "BreakPointInfo::BreakPointInfo..const std::string &file, int)\n");
@@ -152,7 +152,7 @@ BreakPointInfo::BreakPointInfo(bool regex, State state,
 BreakPointInfo::BreakPointInfo(bool regex, State state,
                                InterruptType interrupt,
                                const std::string &url)
-    : m_index(0), m_state(state), m_valid(true), m_interrupt(interrupt),
+    : m_index(0), m_state(state), m_valid(true), m_interruptType(interrupt),
       m_line1(0), m_line2(0), m_url(url),
       m_regex(regex), m_check(false) {
   TRACE(2, "BreakPointInfo::BreakPointInfo..const std::string &url)\n");
@@ -163,16 +163,13 @@ BreakPointInfo::BreakPointInfo(bool regex, State state,
                                InterruptType interrupt,
                                const std::string &exp,
                                const std::string &file)
-    : m_index(0), m_state(state), m_valid(true), m_interrupt(interrupt),
+    : m_index(0), m_state(state), m_valid(true), m_interruptType(interrupt),
       m_line1(0), m_line2(0),
       m_regex(regex), m_check(false) {
   TRACE(2, "BreakPointInfo::BreakPointInfo..const std::string &file)\n");
-  assert(m_interrupt != ExceptionHandler); // Server-side only.
-  if (m_interrupt == ExceptionThrown) {
+  assert(m_interruptType != ExceptionHandler); // Server-side only.
+  if (m_interruptType == ExceptionThrown) {
     parseExceptionThrown(exp);
-    if (!m_file.empty() || m_line1 || m_line2 || !m_funcs.empty()) {
-      m_valid = false;
-    }
   } else {
     parseBreakPointReached(exp, file);
   }
@@ -194,8 +191,8 @@ BreakPointInfo::~BreakPointInfo() {
 
 void BreakPointInfo::sendImpl(DebuggerThriftBuffer &thrift) {
   TRACE(2, "BreakPointInfo::sendImpl\n");
-  thrift.write(m_state);
-  thrift.write(m_interrupt);
+  thrift.write((int8_t)m_state);
+  thrift.write((int8_t)m_interruptType);
   thrift.write(m_file);
   thrift.write(m_line1);
   thrift.write(m_line2);
@@ -213,8 +210,9 @@ void BreakPointInfo::sendImpl(DebuggerThriftBuffer &thrift) {
 
 void BreakPointInfo::recvImpl(DebuggerThriftBuffer &thrift) {
   TRACE(2, "BreakPointInfo::recvImpl\n");
-  thrift.read(m_state);
-  thrift.read(m_interrupt);
+  int8_t tmp;
+  thrift.read(tmp); m_state = (State)tmp;
+  thrift.read(tmp); m_interruptType = (InterruptType)tmp;
   thrift.read(m_file);
   thrift.read(m_line1);
   thrift.read(m_line2);
@@ -281,7 +279,7 @@ void BreakPointInfo::toggle() {
 bool BreakPointInfo::valid() {
   TRACE(2, "BreakPointInfo::valid\n");
   if (m_valid) {
-    switch (m_interrupt) {
+    switch (m_interruptType) {
       case BreakPointReached:
         if (!getFuncName().empty()) {
           if (!m_file.empty() || m_line1 != 0) {
@@ -302,6 +300,8 @@ bool BreakPointInfo::valid() {
       case RequestEnded:
       case PSPEnded:
         return true;
+      default:
+        break;
     }
   }
   return false;
@@ -314,7 +314,7 @@ bool BreakPointInfo::same(BreakPointInfoPtr bpi) {
 
 bool BreakPointInfo::match(InterruptType interrupt, InterruptSite &site) {
   TRACE(2, "BreakPointInfo::match\n");
-  if (m_interrupt == interrupt) {
+  if (m_interruptType == interrupt) {
     switch (interrupt) {
       case RequestStarted:
       case RequestEnded:
@@ -488,7 +488,7 @@ std::string BreakPointInfo::descExceptionThrown() const {
 std::string BreakPointInfo::desc() const {
   TRACE(2, "BreakPointInfo::desc\n");
   string ret;
-  switch (m_interrupt) {
+  switch (m_interruptType) {
     case BreakPointReached:
       ret = descBreakPointReached();
       break;
@@ -496,7 +496,7 @@ std::string BreakPointInfo::desc() const {
       ret = descExceptionThrown();
       break;
     default:
-      ret = GetInterruptName((InterruptType)m_interrupt);
+      ret = GetInterruptName((InterruptType)m_interruptType);
       break;
   }
 

hphp/runtime/debugger/break_point.h

@@ -22,21 +22,37 @@
 namespace HPHP { namespace Eval {
 ///////////////////////////////////////////////////////////////////////////////
 
-enum InterruptType {
+// The type of interrupt that is sent from the server to notify the debugger
+// client about a notable event during execution.
+enum InterruptType : int8_t {
+  // The server is now ready to interact with the debugger
   SessionStarted,
+  // The server has terminated the debug session.
   SessionEnded,
+  // The server has received a web request
   RequestStarted,
+  // The server has sent a response to the web request
   RequestEnded,
+  // The server has finished all processing of a web request
+  // also known as Post Send Processing has Ended.
   PSPEnded,
-  HardBreakPoint, // From f_hphpd_break().
-  BreakPointReached, // Break from the VM interpreter loop
+  // The server has executed f_hphpd_break()
+  HardBreakPoint,
+  // The server has reached a point where it has been told to stop and wait
+  // for the debugger to tell it to resume execution. For example,
+  // a user breakpoint has been reached, or a step command has completed.
+  BreakPointReached,
+  // The server is about throw an exception
   ExceptionThrown,
 
-  // Interrupts for exception handler entry are, for now, server-side only and
-  // only for flow control. We could consider exposing this to clients, and
-  // allowing it as a break point much like ExceptionThrown is, but the value
-  // seems quite low at this time. We assert this stays server-side.
+  // The server has reached the start of an exception handler.
   ExceptionHandler,
+  // The above type of interrupt is not sent from the server to the debugger
+  // but is used for flow control inside the server. We could consider exposing
+  // this type of interrupt to clients, and thus allowing users to request the
+  // server to break execution when an interrupt handler is reached, but the
+  // value seems quite low at this time.
+  // We have assertions that check that these interrupts stays server-side.
 };
 
 // Represents a site in the code, at the source level.
@@ -99,10 +115,11 @@ DECLARE_BOOST_TYPES(DFunctionInfo);
 DECLARE_BOOST_TYPES(BreakPointInfo);
 class BreakPointInfo {
 public:
-  enum State {
-    Always   = -1,
-    Once     = 1,
-    Disabled = 0,
+  // The state of break point
+  enum State : int8_t {
+    Always   = -1, // always break when reaching this break point
+    Once     = 1, // break the first time, then disable
+    Disabled = 0, // carry on with execution when reaching this break point
   };
 
   static const char *ErrorClassName;
@@ -133,6 +150,7 @@ public:
   int index() const { return m_index;}
   std::string state(bool padding) const;
   std::string desc() const;
+
   std::string site() const;
   std::string regex(const std::string &name) const;
 
@@ -151,9 +169,9 @@ public:
 
   int16_t m_index; // client side index number
 
-  int8_t m_state;
-  bool m_valid;
-  int8_t m_interrupt;
+  State m_state; // Always, Once, Disabled
+  bool m_valid; // false if syntactically invalid
+  InterruptType m_interruptType; // Why this break point was reached
 
   // file::line1-line2
   std::string m_file;

hphp/runtime/debugger/cmd/cmd_break.cpp

@@ -495,7 +495,7 @@ void CmdBreak::setClientOutput(DebuggerClient &client) {
     Array breakpoint;
     breakpoint.set(s_id, bpi->index());
     breakpoint.set(s_state, bpi->state(false));
-    if (bpi->m_interrupt == ExceptionThrown) {
+    if (bpi->m_interruptType == ExceptionThrown) {
       breakpoint.set(s_is_exception, true);
       breakpoint.set(s_exception_class, bpi->getExceptionClass());
     } else {