Add comments and split function for better readability.

As part of reading through the code of the list command, I've added some more comments and did a small refactoring to improve readability.
2013-05-17 17:14:40 -07:00
commit 23d9095d4b
@@ -38,15 +38,14 @@ bool CmdMachine::help(DebuggerClient &client) {
  client.helpBody(
    ""
  );
-  return true;
 }

-bool CmdMachine::onClient(DebuggerClient &client) {
-  if (DebuggerCommand::onClient(client)) return true;
+void CmdMachine::onClientImpl(DebuggerClient &client) {
+  if (DebuggerCommand::displayedHelp(client)) return true;

  //TODO

-  return help(client);
+  help(client);
 }

 bool CmdMachine::onServer(DebuggerProxy &proxy) {
@@ -29,10 +29,10 @@ public:

  virtual void list(DebuggerClient &client);
  virtual bool help(DebuggerClient &client);
-
-  virtual bool onClient(DebuggerClient &client);
  virtual bool onServer(DebuggerProxy &proxy);

+protected:
+  virtual void onClientImpl(DebuggerClient &client);
  virtual void sendImpl(DebuggerThriftBuffer &thrift);
  virtual void recvImpl(DebuggerThriftBuffer &thrift);

@@ -24,7 +24,8 @@ namespace HPHP { namespace Eval {

 TRACE_SET_MOD(debugger);

-// Serializes this command into the given Thrift buffer.
+// Always called from send and implements specific
+// logic for serializing a list command to send via Thrift.
 void CmdList::sendImpl(DebuggerThriftBuffer &thrift) {
  DebuggerCommand::sendImpl(thrift);
  thrift.write(m_file);
@@ -33,7 +34,8 @@ void CmdList::sendImpl(DebuggerThriftBuffer &thrift) {
  thrift.write(m_code);
 }

-// Deserializes a CmdList from the given Thrift buffer.
+// Always called from recv and implements specific
+// logic for deserializing a list command received via Thrift.
 void CmdList::recvImpl(DebuggerThriftBuffer &thrift) {
  DebuggerCommand::recvImpl(thrift);
  thrift.read(m_file);
@@ -49,7 +51,7 @@ void CmdList::list(DebuggerClient &client) {
  client.addCompletion(DebuggerClient::AutoCompleteFileNames);
 }

-// The text to display when the debugger client processes "help break".
+// The text to display when the debugger client processes "help list".
 void CmdList::help(DebuggerClient &client) {
  client.helpTitle("List Command");
  client.helpCmds(
@@ -83,31 +85,56 @@ void CmdList::help(DebuggerClient &client) {
  );
 }

-bool CmdList::listCurrent(DebuggerClient &client, int &line,
+// Retrieves the current source location (file, line).
+// The current location is initially determined by the location
+// where execution was interrupted to hand control back to
+// the debugger client and can thereafter be modified by list
+// commands and by switching the the stack frame.
+//
+// The lineFocus and and charFocus parameters
+// are non zero only when the source location comes from a breakpoint.
+// They can be used to highlight the location of the current breakpoint
+// in the edit window of an attached IDE, for example.
+//
+// If m_line1 and m_line2 are currently 0 (because they have not been specified
+// as parameters to the list command), they are updated to point to a block of
+// code one line beyond the current line maintained by the client.
+// This has the effect that a succession of list commands that specify no
+// parameters will scroll sequentially through the source code in blocks
+// of DebuggerClient::CodeBlockSize.
+void CmdList::getListLocation(DebuggerClient &client, int &lineFocus0,
                          int &charFocus0, int &lineFocus1,
                          int &charFocus1) {
-  int linePrev = 0;
-  client.getListLocation(m_file, linePrev, line, charFocus0, lineFocus1,
-                          charFocus1);
+  int currentLine = 0;
+  client.getListLocation(m_file, currentLine, lineFocus0, charFocus0,
+                         lineFocus1, charFocus1);
  if (m_line1 == 0 && m_line2 == 0) {
-    m_line1 = linePrev + 1;
+    m_line1 = currentLine + 1;
    m_line2 = m_line1 + DebuggerClient::CodeBlockSize;
  }
-  if (m_file.empty()) {
-    string code = client.getCode();
-    if (code.empty()) {
-      client.error("There is no current source file.");
-      return true;
-    }
-    client.print(highlight_php(code));
-    return true;
-  }
-  return false;
 }

-bool CmdList::listFileRange(DebuggerClient &client, int line,
-                            int charFocus0, int lineFocus1,
-                            int charFocus1) {
+// If there is no current file, print the desired range of eval code
+// or give an error message if the debugger is not currently performing
+// an eval command.
+void CmdList::listEvalCode(DebuggerClient &client) {
+  assert(m_file.empty());
+
+  string evalCode = client.getCode();
+  if (evalCode.empty()) {
+    client.error("There is no current source file.");
+  } else {
+    client.print(highlight_php(evalCode));
+  }
+}
+
+// Sends this list command to the server to retrieve the source to be listed
+// and then displays the source on the client. The client's current line
+// is then updated to point to the last listed line.
+// Returns false if the server was unable to return source for this command.
+bool CmdList::listFileRange(DebuggerClient &client,
+                            int lineFocus0, int charFocus0,
+                            int lineFocus1, int charFocus1) {
  if (m_line1 <= 0) m_line1 = 1;
  if (m_line2 <= 0) m_line2 = 1;
  if (m_line1 > m_line2) {
@@ -118,7 +145,7 @@ bool CmdList::listFileRange(DebuggerClient &client, int line,

  CmdListPtr res = client.xend<CmdList>(this);
  if (res->m_code.isString()) {
-    if (!client.code(res->m_code, line, m_line1, m_line2, charFocus0,
+    if (!client.code(res->m_code, m_line1, m_line2, lineFocus0, charFocus0,
                      lineFocus1, charFocus1)) {
      client.info("No more lines in %s to display.", m_file.c_str());
    }
@@ -134,6 +161,13 @@ static const StaticString
  s_line1("line2"),
  s_line2("line2");

+// Sends an Info command to the server to retrieve source location
+// information for the function or class specified by the command
+// argument. Then updates this command with the source information
+// and sends it to the server in order to retrieve the source
+// text from the server.
+// Returns false if the server was unable to return the information
+// needed for this command.
 bool CmdList::listFunctionOrClass(DebuggerClient &client) {
  assert(client.argCount() == 1);
  CmdInfoPtr cmdInfo(new CmdInfo());
@@ -163,15 +197,17 @@ bool CmdList::listFunctionOrClass(DebuggerClient &client) {
  int charFocus1 = 0;
  m_file.clear();
  m_line1 = m_line2 = 0;
-  if (listCurrent(client, line, charFocus0, lineFocus1, charFocus1)) {
+  getListLocation(client, line, charFocus0, lineFocus1, charFocus1);
+  if (m_file.empty()) {
+    listEvalCode(client);
    return true;
  }
-  if (listFileRange(client, line, charFocus0, lineFocus1, charFocus1)) {
-    return true;
-  }
-  return false;
+  return listFileRange(client, line, charFocus0, lineFocus1, charFocus1);
 }

+// Checks the command arguments, report errors and returning as appropriate.
+// Then communicates with the server to retrieve source information. Also
+// retrieves and updates location information stored in the client.
 void CmdList::onClientImpl(DebuggerClient &client) {
  if (DebuggerCommand::displayedHelp(client)) return;
  if (client.argCount() > 1) {
@@ -198,7 +234,6 @@ void CmdList::onClientImpl(DebuggerClient &client) {
      }
      return;
    } else {
-
      size_t pos = arg.find(':');
      if (pos != string::npos) {
        m_file = arg.substr(0, pos);
@@ -269,7 +304,9 @@ void CmdList::onClientImpl(DebuggerClient &client) {
  int charFocus1 = 0;

  if (m_file.empty()) {
-    if (listCurrent(client, line, charFocus0, lineFocus1, charFocus1)) {
+    getListLocation(client, line, charFocus0, lineFocus1, charFocus1);
+    if (m_file.empty()) {
+      listEvalCode(client);
      return;
    }
  } else if (m_file[0] == '/') {
@@ -291,6 +328,12 @@ void CmdList::onClientImpl(DebuggerClient &client) {
  }
 }

+// Tries to read the contents of the file whose path is specified in m_file.
+// If the path cannot be resolved and is relative, the path of the sandbox
+// is used to qualify the relative path. If the contents cannot be retrieved
+// m_code will be an empty string.
+// The function returns false if the reply to the client fails during the
+// sending process.
 bool CmdList::onServer(DebuggerProxy &proxy) {
  m_code = f_file_get_contents(m_file.c_str());
  if (!m_code && m_file[0] != '/') {
@@ -26,40 +26,28 @@ DECLARE_BOOST_TYPES(CmdList);
 class CmdList : public DebuggerCommand {
 public:
  CmdList() : DebuggerCommand(KindOfList) {}
-
-  // Sends a "list file" command to the proxy attached to the given client.
-  // Returns false if the file does not exist or could not be read or an
-  // HPHP::String instance containing the contents of the file.
  static Variant GetSourceFile(DebuggerClient &client,
                               const std::string &file);
-
-  // Informs the client of all strings that may follow a list command.
-  // Used for auto completion. The client uses the prefix of the argument
-  // following the command to narrow down the list displayed to the user.
  virtual void list(DebuggerClient &client);
-
-  // The text to display when the debugger client processes "help break".
  virtual void help(DebuggerClient &client);
-
-  // Puts the specified range of the contents of the source file referenced
-  // by this command in m_code and sends a copy of the updated command back
-  // to the client.
  virtual bool onServer(DebuggerProxy &proxy);

 protected:
-  // Verifies the arguments of this command, sends the command to the
-  // server to get back the listing, updates the client with the current
-  // position in the source file and displays a list of source lines to
-  // the console.
  virtual void onClientImpl(DebuggerClient &client);

-  // Serializes this command into the given Thrift buffer.
  virtual void sendImpl(DebuggerThriftBuffer &thrift);
-
-  // Deserializes a CmdList from the given Thrift buffer.
  virtual void recvImpl(DebuggerThriftBuffer &thrift);

 private:
+  void getListLocation(DebuggerClient &client, int &line,
+                            int &charFocus0, int &lineFocus1,
+                            int &charFocus1);
+  void listEvalCode(DebuggerClient &client);
+  bool listFileRange(DebuggerClient &client, int line,
+                     int charFocus0, int lineFocus1,
+                     int charFocus1);
+  bool listFunctionOrClass(DebuggerClient &client);
+
  // A path to a source file. If relative this is relative to url
  // loaded into the server (if any).
  std::string m_file;
@@ -75,16 +63,6 @@ private:
  // range of source text to be listed by this command.
  Variant m_code;

-  bool listCurrent(DebuggerClient &client, int &line,
-                   int &charFocus0, int &lineFocus1,
-                   int &charFocus1);
-
-  bool listFileRange(DebuggerClient &client, int line,
-                     int charFocus0, int lineFocus1,
-                     int charFocus1);
-
-  bool listFunctionOrClass(DebuggerClient &client);
-
 };

 ///////////////////////////////////////////////////////////////////////////////
@@ -1231,8 +1231,8 @@ void DebuggerClient::shortCode(BreakPointInfoPtr bp) {
        }
      }

-      code(source, beginHighlightLine,
-           firstLine, lastLine,
+      code(source, firstLine, lastLine,
+           beginHighlightLine,
           beginHighlightColumn,
           endHighlightLine,
           endHighlightColumn);
@@ -1240,8 +1240,8 @@ void DebuggerClient::shortCode(BreakPointInfoPtr bp) {
  }
 }

-bool DebuggerClient::code(CStrRef source, int lineFocus, int line1 /* = 0 */,
-                          int line2 /* = 0 */, int charFocus0 /* = 0 */,
+bool DebuggerClient::code(CStrRef source, int line1 /*= 0*/, int line2 /*= 0*/,
+                          int lineFocus0 /* = 0 */, int charFocus0 /* = 0 */,
                          int lineFocus1 /* = 0 */, int charFocus1 /* = 0 */) {
  TRACE(2, "DebuggerClient::code\n");
  if (line1 == 0 && line2 == 0) {
@@ -1249,7 +1249,7 @@ bool DebuggerClient::code(CStrRef source, int lineFocus, int line1 /* = 0 */,
    if (isApiMode()) {
      highlighted = source;
    } else {
-      highlighted = highlight_code(source, 0, lineFocus, charFocus0,
+      highlighted = highlight_code(source, 0, lineFocus0, charFocus0,
                                   lineFocus1, charFocus1);
    }
    if (!highlighted.empty()) {
@@ -1263,7 +1263,7 @@ bool DebuggerClient::code(CStrRef source, int lineFocus, int line1 /* = 0 */,
  if (isApiMode()) {
    highlighted = source;
  } else {
-    highlighted = highlight_php(source, 1, lineFocus, charFocus0,
+    highlighted = highlight_php(source, 1, lineFocus0, charFocus0,
                                lineFocus1, charFocus1);
  }
  int line = 1;
@@ -1992,11 +1992,14 @@ DThreadInfoPtr DebuggerClient::getThread(int index) const {
  return DThreadInfoPtr();
 }

-// Retrieves a source location that is the current focus of the
-// debugger. The current focus is initially determined by the
+// Retrieves the current source location (file, line).
+// The current location is initially determined by the
 // breakpoint where the debugger is currently stopped and can
 // thereafter be modified by list commands and by switching the
-// the stack frame.
+// the stack frame. The lineFocus and and charFocus parameters
+// are non zero only when the source location comes from a breakpoint.
+// They can be used to highlight the location of the current breakpoint
+// in the edit window of an attached IDE, for example.
 void DebuggerClient::getListLocation(std::string &file, int &line,
                                     int &lineFocus0, int &charFocus0,
                                     int &lineFocus1, int &charFocus1) {
@@ -420,7 +420,12 @@ private:
  InstPointInfoPtrVec m_instPoints;

  // list command's current location, which may be different from m_breakpoint
+
+  // The file currently being listed. Set implicitly by breakpoints and
+  // explicitly by list commands issued to the client by a user or via the API.
  std::string m_listFile;
+
+  // The first line to list
  int m_listLine;
  int m_listLineFocus;

@@ -25,7 +25,9 @@ namespace HPHP { namespace Eval {
 ///////////////////////////////////////////////////////////////////////////////
 TRACE_SET_MOD(debugger);

-// send/recv
+// Resets the buffer, serializes this command into the buffer and then
+// flushes the buffer.
+// Returns false if an exception occurs during these steps.
 bool DebuggerCommand::send(DebuggerThriftBuffer &thrift) {
  TRACE(5, "DebuggerCommand::send\n");
  try {
@@ -50,6 +52,8 @@ bool DebuggerCommand::recv(DebuggerThriftBuffer &thrift) {
  return true;
 }

+// Always called from send and must implement the subclass specific
+// logic for serializing a command to send via Thrift.
 void DebuggerCommand::sendImpl(DebuggerThriftBuffer &thrift) {
  TRACE(5, "DebuggerCommand::sendImpl\n");
  thrift.write((int32_t)m_type);
@@ -58,6 +62,8 @@ void DebuggerCommand::sendImpl(DebuggerThriftBuffer &thrift) {
  thrift.write(m_version);
 }

+// Always called from recv and must implement the subclass specific
+// logic for deserializing a command received via Thrift.
 void DebuggerCommand::recvImpl(DebuggerThriftBuffer &thrift) {
  TRACE(5, "DebuggerCommand::recvImpl\n");
  thrift.read(m_body);
@@ -145,10 +151,15 @@ bool DebuggerCommand::Receive(DebuggerThriftBuffer &thrift,
 ///////////////////////////////////////////////////////////////////////////////
 // default handlers

+// Informs the client of all argument strings that may follow this command
+// name. Used for auto completion. The client uses the prefix of the argument
+// following the command name to narrow down the list displayed to the user.
 void DebuggerCommand::list(DebuggerClient &client) {
  TRACE(2, "DebuggerCommand::list\n");
 }

+// The text to display when the debugger client
+// processes "help <this command name>".
 void DebuggerCommand::help(DebuggerClient &client) {
  TRACE(2, "DebuggerCommand::help\n");
  assert(false);
@@ -166,6 +177,10 @@ bool DebuggerCommand::displayedHelp(DebuggerClient &client) {
  return false;
 }

+// Carries out the command, possibly by sending it to the server.
+// If the client is controlled via the API, the setClientOuput method
+// is invoked to update the client with the command output for access
+// via the API.
 void DebuggerCommand::onClient(DebuggerClient &client) {
  TRACE(2, "DebuggerCommand::onClient\n");
  onClientImpl(client);
@@ -174,12 +189,17 @@ void DebuggerCommand::onClient(DebuggerClient &client) {
  }
 }

+// Updates the client with information about the execution of this command.
+// This information is not used by the command line client, but can
+// be accessed via the debugger client API exposed to PHP programs.
 void DebuggerCommand::setClientOutput(DebuggerClient &client) {
  TRACE(2, "DebuggerCommand::setClientOutput\n");
  // Just default to text
  client.setOutputType(DebuggerClient::OTText);
 }

+// Server-side work for a command. Returning false indicates a failure to
+// communicate with the client (for commands that do so).
 bool DebuggerCommand::onServer(DebuggerProxy &proxy) {
  TRACE(2, "DebuggerCommand::onServer\n");
  assert(false);
@@ -90,35 +90,13 @@ public:
      m_incomplete(false) {}

  bool is(Type type) const { return m_type == type;}
-
  Type getType() const { return m_type;}
-
  bool send(DebuggerThriftBuffer &thrift);
-
  bool recv(DebuggerThriftBuffer &thrift);
-
-  // Informs the client of all strings that may follow a break command.
-  // Used for auto completion. The client uses the prefix of the argument
-  // following the command to narrow down the list displayed to the user.
  virtual void list(DebuggerClient &client);
-
-  // The text to display when the debugger client
-  // processes "help <this command name>".
  virtual void help(DebuggerClient &client);
-
-  // Carries out the command, possibly by sending it to the server.
-  // If the client is controlled via the API, the setClientOuput method
-  // is invoked to update the client with the command output for access
-  // via the API.
  void onClient(DebuggerClient &client);
-
-  // Updates the client with information about the execution of this command.
-  // This information is not used by the command line client, but can
-  // be accessed via the debugger client API exposed to PHP programs.
  virtual void setClientOutput(DebuggerClient &client);
-
-  // Server-side work for a command. Returning false indicates a failure to
-  // communicate with the client (for commands that do so).
  virtual bool onServer(DebuggerProxy &proxy);

  // This seems to be confined to eval and print commands.
@@ -138,17 +116,8 @@ protected:
  // Carries out the command, possibly by sending it to the server.
  virtual void onClientImpl(DebuggerClient &client) = 0;

-  // If the first argument of the command is "help" or "?"
-  // this displays help text for the command and returns true.
-  // Otherwise it returns false.
  bool displayedHelp(DebuggerClient &client);
-
-  // Always called from send and must implement the subclass specific
-  // logic for serializing a command to send via Thrift.
  virtual void sendImpl(DebuggerThriftBuffer &thrift);
-
-  // Always called from recv and must implement the subclass specific
-  // logic for deserializing a command received via Thrift.
  virtual void recvImpl(DebuggerThriftBuffer &thrift);

  Type m_type;
@@ -311,6 +311,9 @@ void DebuggerProxy::interrupt(CmdInterrupt &cmd) {
  }
 }

+// Sends a copy of the given command to the associated client
+// using the buffer in m_thrift. Returns false if an exception
+// occurs during the send (typically a socket error).
 bool DebuggerProxy::sendToClient(DebuggerCommand *cmd) {
  TRACE(2, "DebuggerProxy::sendToClient\n");
  return cmd->send(m_thrift);
@@ -229,7 +229,7 @@ void c_DebuggerClientCmdUser::t_error(int _argc, CStrRef format,
 void c_DebuggerClientCmdUser::t_code(CStrRef source, int highlight_line /* = 0 */,
                              int start_line_no /* = 0 */,
                              int end_line_no /* = 0 */) {
-  m_client->code(source, highlight_line, start_line_no, end_line_no);
+  m_client->code(source, start_line_no, end_line_no, highlight_line);
 }

 Variant c_DebuggerClientCmdUser::t_ask(int _argc, CStrRef format,