Reorganize CommandLine code.

base/command_line.h

Index: base/command_line.h
diff --git a/base/command_line.h b/base/command_line.h
index db6145879a61120bf87fd4fa5544f797f2197b4c..ee8112a545767dca8c965b5d8841f6356348b667 100644
--- a/base/command_line.h
+++ b/base/command_line.h
@@ -3,15 +3,13 @@
 // found in the LICENSE file.
 
 // This class works with command lines: building and parsing.
-// Switches can optionally have a value attached using an equals sign,
-// as in "-switch=value".  Arguments that aren't prefixed with a
-// switch prefix are saved as extra arguments.  An argument of "--"
-// will terminate switch parsing, causing everything after to be
-// considered as extra arguments.
+// Switches can optionally have a value attached using an equals sign, as in
+// "-switch=value". Arguments that aren't prefixed with a switch prefix are
+// saved as extra arguments. An argument of "--" will terminate switch parsing,
+// causing everything after to be considered as extra arguments.
 
-// There is a singleton read-only CommandLine that represents the command
-// line that the current process was started with.  It must be initialized
-// in main() (or whatever the platform's equivalent function is).
+// There is a singleton read-only CommandLine that represents the command line
+// that the current process was started with.  It must be initialized in main().
 
 #ifndef BASE_COMMAND_LINE_H_
 #define BASE_COMMAND_LINE_H_
@@ -25,111 +23,89 @@
 #include "build/build_config.h"
 
 class FilePath;
-class InProcessBrowserTest;
 
 class CommandLine {
  public:
 #if defined(OS_WIN)
-  // The type of native command line arguments.
+  // The native command line string type.
   typedef std::wstring StringType;
 #elif defined(OS_POSIX)
-  // The type of native command line arguments.
   typedef std::string StringType;
 #endif
 
+  typedef std::vector<StringType> StringVector;
   // The type of map for parsed-out switch key and values.
   typedef std::map<std::string, StringType> SwitchMap;
 
-  // A constructor for CommandLines that are used only to carry switches and
-  // arguments.
+  // A constructor for CommandLines that only carry switches and arguments.
   enum NoProgram { NO_PROGRAM };
   explicit CommandLine(NoProgram no_program);
 
-  // Construct a new, empty command line.
-  // |program| is the name of the program to run (aka argv[0]).
+  // Construct a new command line with |program| as argv[0].
   explicit CommandLine(const FilePath& program);
 
 #if defined(OS_POSIX)
   CommandLine(int argc, const char* const* argv);
-  explicit CommandLine(const std::vector<std::string>& argv);
+  explicit CommandLine(const StringVector& argv);
 #endif
 
-  ~CommandLine();
-
-#if defined(OS_WIN)
-  // Initialize by parsing the given command-line string.
-  // The program name is assumed to be the first item in the string.
-  void ParseFromString(const std::wstring& command_line);
-  static CommandLine FromString(const std::wstring& command_line);
-#elif defined(OS_POSIX)
-  // Initialize from an argv vector.
-  void InitFromArgv(int argc, const char* const* argv);
-  void InitFromArgv(const std::vector<std::string>& argv);
-#endif
-
-  // Initialize the current process CommandLine singleton.  On Windows,
-  // ignores its arguments (we instead parse GetCommandLineW()
-  // directly) because we don't trust the CRT's parsing of the command
-  // line, but it still must be called to set up the command line.
+  // Initialize the current process CommandLine singleton. On Windows, ignores
+  // its arguments (we instead parse GetCommandLineW() directly) because we
+  // don't trust the CRT's parsing of the command line, but it still must be
+  // called to set up the command line.
   static void Init(int argc, const char* const* argv);
 
   // Destroys the current process CommandLine singleton. This is necessary if
-  // you want to reset the base library to its initial state (for example in an
+  // you want to reset the base library to its initial state (for example, in an
   // outer library that needs to be able to terminate, and be re-initialized).
-  // If Init is called only once, e.g. in main(), calling Reset() is not
-  // necessary.
+  // If Init is called only once, as in main(), Reset() is not necessary.
   static void Reset();
 
   // Get the singleton CommandLine representing the current process's
-  // command line.  Note: returned value is mutable, but not thread safe;
+  // command line. Note: returned value is mutable, but not thread safe;
   // only mutate if you know what you're doing!
   static CommandLine* ForCurrentProcess();
 
-  // Returns true if this command line contains the given switch.
-  // (Switch names are case-insensitive.)
-  bool HasSwitch(const std::string& switch_string) const;
-
-  // Returns the value associated with the given switch.  If the
-  // switch has no value or isn't present, this method returns
-  // the empty string.
-  std::string GetSwitchValueASCII(const std::string& switch_string) const;
-  FilePath GetSwitchValuePath(const std::string& switch_string) const;
-  StringType GetSwitchValueNative(const std::string& switch_string) const;
-
-  // Get the number of switches in this process.
-  size_t GetSwitchCount() const { return switches_.size(); }
+#if defined(OS_WIN)
+  static CommandLine FromString(const StringType& command_line);

[evanm, 2011/02/28 21:04:19]

Could use std::wstring here, might be clearer.  Like how you use char* below for
argv.

Edit: I see you're switching to StringType throughout.  I guess it might make
switching to string16 easier.  I feel like it's usually clearer to not need to
read through typedefs for this kind of thing but I'll defer to your judgement.

[msw, 2011/02/28 23:44:47]

I'll use string16 everywhere and remove typedefs (PKasting's suggestion) in
another change. I reverted this function for now. What are your thoughts on that
plan, and do you dislike StringVector/SwitchMap?

+#endif
 
-  // Get a copy of all switches, along with their values
-  const SwitchMap& GetSwitches() const {
-    return switches_;
-  }
+#if defined(OS_POSIX)
+  // Initialize from an argv vector.
+  void InitFromArgv(int argc, const char* const* argv);
+  void InitFromArgv(const StringVector& argv);
+#endif
 
-  // Get the remaining arguments to the command.
-  const std::vector<StringType>& args() const { return args_; }
+  // Returns the represented command line string.

[evanm, 2011/02/28 21:04:19]

Can you stick a note on this about how it shouldn't be used because the quoting
is unclear?

[msw, 2011/02/28 23:44:47]

Done.

+  StringType command_line_string() const;
 
-#if defined(OS_WIN)
-  // Returns the original command line string.
-  const std::wstring& command_line_string() const {
-    return command_line_string_;
-  }
-#elif defined(OS_POSIX)
+#if defined(OS_POSIX)
   // Returns the original command line string as a vector of strings.
-  const std::vector<std::string>& argv() const {
-    return argv_;
-  }
-  // Try to match the same result as command_line_string() would get you
-  // on windows.
-  std::string command_line_string() const;
+  const StringVector& argv() const { return argv_; }
 #endif
 
   // Returns the program part of the command line string (the first item).
   FilePath GetProgram() const;
 
-  // Append a switch to the command line.
-  void AppendSwitch(const std::string& switch_string);
+  // Returns true if this command line contains the given switch.
+  // (Switch names are case-insensitive).
+  bool HasSwitch(const std::string& switch_string) const;
+
+  // Returns the value associated with the given switch. If the switch has no
+  // value or isn't present, this method returns the empty string.
+  std::string GetSwitchValueASCII(const std::string& switch_string) const;
+  FilePath GetSwitchValuePath(const std::string& switch_string) const;
+  StringType GetSwitchValueNative(const std::string& switch_string) const;
+
+  // Get the number of switches in this process.
+  size_t GetSwitchCount() const;

[evanm, 2011/02/28 21:04:19]

Ugh, I wish we didn't have all this API.  This could just be
GetSwitches().size(); any caller that needs to know the switch count probably
wants to iterate through the map anyway.  Something to change later.

[msw, 2011/02/28 23:44:47]

Agreed; added // TODO(msw): Remove unnecessary API.

 
-  // Append a switch and value to the command line.
+  // Get a copy of all switches, along with their values.
+  const SwitchMap& GetSwitches() const { return switches_; }
+
+  // Append a switch [with optional value] to the command line.
   // CAUTION! Appending a switch after the "--" kSwitchTerminator is futile!

[evanm, 2011/02/28 21:04:19]

It appears that kSwitchTerminator doesn't exist in this header.  Might be
clearer to just call it "switch terminator" or somethign.

[msw, 2011/02/28 23:44:47]

Done.

+  void AppendSwitch(const std::string& switch_string);
   void AppendSwitchPath(const std::string& switch_string, const FilePath& path);
   void AppendSwitchNative(const std::string& switch_string,
                           const StringType& value);
@@ -137,11 +113,17 @@ class CommandLine {
                          const std::string& value);
   void AppendSwitches(const CommandLine& other);
 
-  // Append an argument to the command line.
-  // Note on quoting: the argument will be quoted properly such that it is
-  // interpreted as one argument to the target command.
-  // AppendArg is primarily for ASCII; non-ASCII input will be
-  // interpreted as UTF-8.
+  // Copy a set of switches (and any values) from another command line.
+  // Commonly used when launching a subprocess.
+  void CopySwitchesFrom(const CommandLine& source, const char* const switches[],
+                        size_t count);
+
+  // Get the remaining arguments to the command.
+  const StringVector& args() const { return args_; }
+
+  // Append an argument to the command line. Note that the argument is quoted
+  // properly such that it is interpreted as one argument to the target command.
+  // AppendArg is primarily for ASCII; non-ASCII input is interpreted as UTF-8.
   void AppendArg(const std::string& value);
   void AppendArgPath(const FilePath& value);
   void AppendArgNative(const StringType& value);
@@ -152,57 +134,45 @@ class CommandLine {
   void AppendArguments(const CommandLine& other,
                        bool include_program);
 
-  // Insert a command before the current command.  Common for debuggers,
-  // like "valgrind" or "gdb --args".
+  // Insert a command before the current command.
+  //Common for debuggers, like "valgrind" or "gdb --args".

[evanm, 2011/02/28 21:04:19]

Space after //

[msw, 2011/02/28 23:44:47]

Done.

   void PrependWrapper(const StringType& wrapper);
 
-  // Copy a set of switches (and their values, if any) from another command
-  // line.  Commonly used when launching a subprocess.
-  void CopySwitchesFrom(const CommandLine& source, const char* const switches[],
-                        size_t count);
+#if defined(OS_WIN)
+  // Initialize by parsing the given command line string.
+  // The program name is assumed to be the first item in the string.
+  void ParseFromString(const std::wstring& command_line);
+#endif
 
  private:
-  friend class InProcessBrowserTest;
-
-  CommandLine();
+  // Disallow default constructor; a program name must be explicitly specified.
+  CommandLine() {}
 
-  // Used by InProcessBrowserTest.
-  static CommandLine* ForCurrentProcessMutable();
-
-  // Returns true and fills in |switch_string| and |switch_value|
-  // if |parameter_string| represents a switch.
-  static bool IsSwitch(const StringType& parameter_string,
-                       std::string* switch_string,
-                       StringType* switch_value);
+  // Allow the copy constructor. A common pattern is to copy the current
+  // process's command line and then add some flags to it. For example:
+  //   CommandLine cl(*CommandLine::ForCurrentProcess());
+  //   cl.AppendSwitch(...);
 
-  // The singleton CommandLine instance representing the current process's
-  // command line.
+  // The singleton CommandLine representing the current process's command line.
   static CommandLine* current_process_commandline_;
 
   // We store a platform-native version of the command line, used when building
-  // up a new command line to be executed.  This ifdef delimits that code.
-
+  // up a new command line to be executed. This ifdef delimits that code.
 #if defined(OS_WIN)
-  // The quoted, space-separated command-line string.
-  std::wstring command_line_string_;
+  // The quoted, space-separated command line string.
+  StringType command_line_string_;
   // The name of the program.
-  std::wstring program_;
+  StringType program_;
 #elif defined(OS_POSIX)
   // The argv array, with the program name in argv_[0].
-  std::vector<std::string> argv_;
+  StringVector argv_;
 #endif
 
-  // Parsed-out values.
+  // Parsed-out switch keys and values.
   SwitchMap switches_;
 
-  // Non-switch command-line arguments.
-  std::vector<StringType> args_;
-
-  // We allow copy constructors, because a common pattern is to grab a

[evanm, 2011/02/28 21:04:19]

I think this comment was ok to leave in.  It's saying why we don't have the
DISALLOW_COPY_AND_ASSIGN that is normally required on all classes.

(FWIW, I'd prefer to get rid of the copy ctor anyway.)

[msw, 2011/02/28 23:44:47]

Done. Was moved above; but I see this is where we normally put
DISALLOW_COPY_AND_ASSIGN, etc. So I moved it back here.

-  // copy of the current process's command line and then add some
-  // flags to it.  E.g.:
-  //   CommandLine cl(*CommandLine::ForCurrentProcess());
-  //   cl.AppendSwitch(...);
+  // Non-switch command line arguments.
+  StringVector args_;
 };
 
 #endif  // BASE_COMMAND_LINE_H_