CommandLine refactoring and cleanup.

base/command_line.cc

Index: base/command_line.cc
diff --git a/base/command_line.cc b/base/command_line.cc
index b027d2a22a121f69de76292750b8965dd4fba108..617ee964dd4b856feb171a73a0f636df757fb5ae 100644
--- a/base/command_line.cc
+++ b/base/command_line.cc
@@ -23,29 +23,45 @@
 CommandLine* CommandLine::current_process_commandline_ = NULL;
 
 namespace {
-typedef CommandLine::StringType::value_type CharType;
-
-const CharType kSwitchTerminator[] = FILE_PATH_LITERAL("--");
-const CharType kSwitchValueSeparator[] = FILE_PATH_LITERAL("=");
+const CommandLine::CharType kSwitchTerminator[] = FILE_PATH_LITERAL("--");
+const CommandLine::CharType kSwitchValueSeparator[] = FILE_PATH_LITERAL("=");
 // Since we use a lazy match, make sure that longer versions (like "--") are
 // listed before shorter versions (like "-") of similar prefixes.
 #if defined(OS_WIN)
-const CharType* const kSwitchPrefixes[] = {L"--", L"-", L"/"};
+const CommandLine::CharType* const kSwitchPrefixes[] = {L"--", L"-", L"/"};
 #elif defined(OS_POSIX)
 // Unixes don't use slash as a switch.
-const CharType* const kSwitchPrefixes[] = {"--", "-"};
+const CommandLine::CharType* const kSwitchPrefixes[] = {"--", "-"};
+#endif
+
+CommandLine::StringType Native(const std::string& string) {

[Evan Martin, 2011/05/10 23:48:32]

Can you make this name more descriptive?  Maybe just ToNative would be better. 
A comment probably would've helped too.  :)

[msw, 2011/05/11 02:28:12]

Done; I renamed this function and added a TODO about string16.

+#if defined(OS_WIN)
+  return ASCIIToWide(string);
+#elif defined(OS_POSIX)
+  return string;
 #endif
+}
 
+CommandLine::StringType Native(const std::wstring& string) {
 #if defined(OS_WIN)
-// Lowercase a string for case-insensitivity of switches.
-// Is this desirable? It exists for backwards compatibility on Windows.
+  return string;
+#elif defined(OS_POSIX)
+  return WideToASCII(string);
+#endif
+}
+
+// Lowercase a string for case-insensitivity of switches *on Windows*.
+// This exists for backwards compatibility on Windows.
 void Lowercase(std::string* arg) {
+#if defined(OS_WIN)
   transform(arg->begin(), arg->end(), arg->begin(), tolower);
+#endif
 }
 
-// Quote a string if necessary, such that CommandLineToArgvW() will always
-// process it as a single argument.
-std::wstring WindowsStyleQuote(const std::wstring& arg) {
+// Quote a string if necessary *on Windows*.
+// This ensures compatibility with CommandLineToArgvW().
+CommandLine::StringType Quote(const CommandLine::StringType& arg) {
+#if defined(OS_WIN)
   // We follow the quoting rules of CommandLineToArgvW.
   // http://msdn.microsoft.com/en-us/library/17w5ykft.aspx
   if (arg.find_first_of(L" \\\"") == std::wstring::npos) {
@@ -85,76 +101,85 @@ std::wstring WindowsStyleQuote(const std::wstring& arg) {
   out.push_back('"');
 
   return out;
-}
+#elif defined(OS_POSIX)
+  // TODO(msw): Proper POSIX quoting.

[Evan Martin, 2011/05/10 23:48:32]

I don't think this can be done.  Quoting is always done relative to a program
that interprets the quoting: for example, in generating our .desktop files (used
to launch the app from an icon) the rules for quoting are different than the
rules for a command line that works in a bash script or the shell.

Similarly, the quoting we do here works for some Windows apps but not
necessarily all.  That is why I emphasized in the comment you deleted that it
works with CommandLineToArgvW() specifically.


In general, I dislike the idea of a generic "quote" function.  I would prefer
for callers to specifically request what sort of quoting they would like (for
example including it in the function name) or just expose API so that they can
do the quoting themselves (like we do for the .desktop files above).

[msw, 2011/05/11 02:28:12]

Thank you for some perspective on this issue. For now, I renamed this function,
pushed the OS_WIN filtering to its uses in command_line_string() (like
Lowercase), removed the TODO, and tweaked the function comment.

+  return arg;
 #endif
+}
 
-// Returns true and fills in |switch_string| and |switch_value| if
-// |parameter_string| represents a switch.
-bool IsSwitch(const CommandLine::StringType& parameter_string,
+size_t GetSwitchPrefixLength(const CommandLine::StringType& string) {
+  for (size_t i = 0; i < arraysize(kSwitchPrefixes); ++i) {
+    CommandLine::StringType prefix(kSwitchPrefixes[i]);
+    if (string.find(prefix) == 0)
+      return prefix.length();
+  }
+  return 0;
+}
+
+// Fills in |switch_string| and |switch_value| if |string| is a switch.
+bool IsSwitch(const CommandLine::StringType& string,
               std::string* switch_string,
               CommandLine::StringType* switch_value) {
   switch_string->clear();
   switch_value->clear();
+  if (GetSwitchPrefixLength(string) == 0)
+    return false;
 
-  for (size_t i = 0; i < arraysize(kSwitchPrefixes); ++i) {
-    CommandLine::StringType prefix(kSwitchPrefixes[i]);
-    if (parameter_string.find(prefix) != 0)
-      continue;
-
-    const size_t switch_start = prefix.length();
-    const size_t equals_position = parameter_string.find(
-        kSwitchValueSeparator, switch_start);
-    CommandLine::StringType switch_native;
-    if (equals_position == CommandLine::StringType::npos) {
-      switch_native = parameter_string.substr(switch_start);
-    } else {
-      switch_native = parameter_string.substr(
-          switch_start, equals_position - switch_start);
-      *switch_value = parameter_string.substr(equals_position + 1);
-    }
+  const size_t equals_position = string.find(kSwitchValueSeparator);
+  CommandLine::StringType switch_native = string.substr(0, equals_position);
 #if defined(OS_WIN)
-    *switch_string = WideToASCII(switch_native);
-    Lowercase(switch_string);
-#else
-    *switch_string = switch_native;
+  *switch_string = WideToASCII(switch_native);
+#elif defined(OS_POSIX)
+  *switch_string = switch_native;
 #endif
+  Lowercase(switch_string);
+  if(equals_position != CommandLine::StringType::npos)
+    *switch_value = string.substr(equals_position + 1);
+  return true;
+}
 
-    return true;
-  }
+void AppendSwitchesAndArguments(CommandLine& command_line,
+                                const CommandLine::StringVector& argv) {
+  // Append switches and arguments, keeping switches before arguments.
+  bool parse_switches = true;
+  for (size_t i = 1; i < argv.size(); ++i) {
+    CommandLine::StringType arg = argv[i];
+    TrimWhitespace(arg, TRIM_ALL, &arg);
 
-  return false;
+    std::string switch_string;
+    CommandLine::StringType switch_value;
+    parse_switches &= arg != kSwitchTerminator;
+    if (parse_switches && IsSwitch(arg, &switch_string, &switch_value))
+      command_line.AppendSwitchNative(switch_string, switch_value);
+    else
+      command_line.AppendArgNative(arg);
+  }
 }
 
 }  // namespace
 
-CommandLine::CommandLine(NoProgram no_program) {
-#if defined(OS_POSIX)
-  // Push an empty argument, because we always assume argv_[0] is a program.
-  argv_.push_back("");
-#endif
+CommandLine::CommandLine(NoProgram no_program)
+    : argv_(1),
+      begin_args_(1) {
 }
 
-CommandLine::CommandLine(const FilePath& program) {
-#if defined(OS_WIN)
-  if (!program.empty()) {
-    program_ = program.value();
-    // TODO(evanm): proper quoting here.
-    command_line_string_ = L'"' + program.value() + L'"';
-  }
-#elif defined(OS_POSIX)
-  argv_.push_back(program.value());
-#endif
+CommandLine::CommandLine(const FilePath& program)
+    : argv_(1),
+      begin_args_(1) {
+  SetProgram(program);
 }
 
-#if defined(OS_POSIX)
-CommandLine::CommandLine(int argc, const char* const* argv) {
+CommandLine::CommandLine(int argc, const CommandLine::CharType* const* argv)
+    : argv_(1),
+      begin_args_(1) {
   InitFromArgv(argc, argv);
 }
 
-CommandLine::CommandLine(const StringVector& argv) {
+CommandLine::CommandLine(const StringVector& argv)
+    : argv_(1),
+      begin_args_(1) {
   InitFromArgv(argv);
 }
-#endif  // OS_POSIX
 
 CommandLine::~CommandLine() {
 }
@@ -162,7 +187,7 @@ CommandLine::~CommandLine() {
 // static
 void CommandLine::Init(int argc, const char* const* argv) {
   delete current_process_commandline_;
-  current_process_commandline_ = new CommandLine;
+  current_process_commandline_ = new CommandLine(NO_PROGRAM);
 #if defined(OS_WIN)
   current_process_commandline_->ParseFromString(::GetCommandLineW());
 #elif defined(OS_POSIX)
@@ -186,74 +211,68 @@ CommandLine* CommandLine::ForCurrentProcess() {
 #if defined(OS_WIN)
 // static
 CommandLine CommandLine::FromString(const std::wstring& command_line) {
-  CommandLine cmd;
+  CommandLine cmd(NO_PROGRAM);
   cmd.ParseFromString(command_line);
   return cmd;
 }
-#endif  // OS_WIN
+#endif
 
-#if defined(OS_POSIX)
-void CommandLine::InitFromArgv(int argc, const char* const* argv) {
+void CommandLine::InitFromArgv(int argc,
+                               const CommandLine::CharType* const* argv) {
+  StringVector new_argv;
   for (int i = 0; i < argc; ++i)
-    argv_.push_back(argv[i]);
-  InitFromArgv(argv_);
+    new_argv.push_back(argv[i]);
+  InitFromArgv(new_argv);
 }
 
 void CommandLine::InitFromArgv(const StringVector& argv) {
-  argv_ = argv;
+  argv_ = StringVector(1);
+  begin_args_ = 1;
+  SetProgram(argv.empty() ? FilePath() : FilePath(argv[0]));
+  AppendSwitchesAndArguments(*this, argv);
+}
+
+CommandLine::StringType CommandLine::command_line_string() const {
+  StringType string(Quote(argv_[0]));
+  // Append switches and arguments.
   bool parse_switches = true;
   for (size_t i = 1; i < argv_.size(); ++i) {
-    const std::string& arg = argv_[i];
-
-    if (!parse_switches) {
-      args_.push_back(arg);
-      continue;
-    }
-
-    if (arg == kSwitchTerminator) {
-      parse_switches = false;
-      continue;
-    }
-
+    CommandLine::StringType arg = argv_[i];
     std::string switch_string;
-    StringType switch_value;
-    if (IsSwitch(arg, &switch_string, &switch_value)) {
-      switches_[switch_string] = switch_value;
-    } else {
-      args_.push_back(arg);
+    CommandLine::StringType switch_value;
+    parse_switches &= argv_[i] != kSwitchTerminator;
+    string.append(StringType(FILE_PATH_LITERAL(" ")));
+    if (parse_switches && IsSwitch(argv_[i], &switch_string, &switch_value)) {
+      string.append(Native(switch_string));
+      if (!switch_value.empty())
+        string.append(kSwitchValueSeparator + Quote(switch_value));
+    }
+    else {
+      string.append(Quote(arg));
     }
   }
+  return string;
 }
-#endif  // OS_POSIX
 
-CommandLine::StringType CommandLine::command_line_string() const {
-#if defined(OS_WIN)
-  return command_line_string_;
-#elif defined(OS_POSIX)
-  return JoinString(argv_, ' ');
-#endif
+FilePath CommandLine::GetProgram() const {
+  return FilePath(program_string_);
 }
 
-FilePath CommandLine::GetProgram() const {
-#if defined(OS_WIN)
-  return FilePath(program_);
-#else
-  DCHECK_GT(argv_.size(), 0U);
-  return FilePath(argv_[0]);
-#endif
+void CommandLine::SetProgram(const FilePath& program) {
+  program_string_ = program.value();
+  TrimWhitespace(program_string_, TRIM_ALL, &program_string_);
+  argv_[0] = program_string_;
 }
 
 bool CommandLine::HasSwitch(const std::string& switch_string) const {
   std::string lowercased_switch(switch_string);
-#if defined(OS_WIN)
   Lowercase(&lowercased_switch);
-#endif
   return switches_.find(lowercased_switch) != switches_.end();
 }
 
 std::string CommandLine::GetSwitchValueASCII(
     const std::string& switch_string) const {
-  CommandLine::StringType value = GetSwitchValueNative(switch_string);
+  StringType value = GetSwitchValueNative(switch_string);
   if (!IsStringASCII(value)) {
     LOG(WARNING) << "Value of --" << switch_string << " must be ASCII.";
     return "";
@@ -273,17 +292,9 @@ FilePath CommandLine::GetSwitchValuePath(
 CommandLine::StringType CommandLine::GetSwitchValueNative(
     const std::string& switch_string) const {
   std::string lowercased_switch(switch_string);
-#if defined(OS_WIN)
   Lowercase(&lowercased_switch);
-#endif

[Evan Martin, 2011/05/10 23:48:32]

Since you have to have this ifdef somewhere, why move it to the other location? 
It seems less readable to me.  (In reading the new code, it looks like we always
lowercase the switch).  If you really prefer this, consider renaming the
function LowercaseOnWindows or something.

[msw, 2011/05/11 02:28:12]

Done (reverted). I was naively going for fewer #ifdefs.

-
   SwitchMap::const_iterator result = switches_.find(lowercased_switch);
-
-  if (result == switches_.end()) {
-    return CommandLine::StringType();
-  } else {
-    return result->second;
-  }
+  return result == switches_.end() ? StringType() : result->second;
 }
 
 size_t CommandLine::GetSwitchCount() const {
@@ -291,14 +302,7 @@ size_t CommandLine::GetSwitchCount() const {
 }
 
 void CommandLine::AppendSwitch(const std::string& switch_string) {
-#if defined(OS_WIN)
-  command_line_string_.append(L" ");
-  command_line_string_.append(kSwitchPrefixes[0] + ASCIIToWide(switch_string));
-  switches_[switch_string] = L"";
-#elif defined(OS_POSIX)
-  argv_.push_back(kSwitchPrefixes[0] + switch_string);
-  switches_[switch_string] = "";
-#endif
+  AppendSwitchNative(switch_string, StringType());
 }
 
 void CommandLine::AppendSwitchPath(const std::string& switch_string,
@@ -308,49 +312,40 @@ void CommandLine::AppendSwitchPath(const std::string& switch_string,
 
 void CommandLine::AppendSwitchNative(const std::string& switch_string,
                                      const CommandLine::StringType& value) {
-#if defined(OS_WIN)
-  StringType combined_switch_string =
-      kSwitchPrefixes[0] + ASCIIToWide(switch_string);
-  if (!value.empty())
-    combined_switch_string += kSwitchValueSeparator + WindowsStyleQuote(value);
-
-  command_line_string_.append(L" ");
-  command_line_string_.append(combined_switch_string);
-
-  switches_[switch_string] = value;
-#elif defined(OS_POSIX)
-  StringType combined_switch_string = kSwitchPrefixes[0] + switch_string;
+  StringType combined_switch_string = Native(switch_string);
+  size_t prexfix_length = GetSwitchPrefixLength(combined_switch_string);

[Evan Martin, 2011/05/10 23:48:32]

typo: "prefix"

[msw, 2011/05/11 02:28:12]

Done.

+  switches_[switch_string.substr(prexfix_length)] = value;
+  // Preserve existing switch prefixes in |argv_|; only append one if necessary.
+  if (prexfix_length == 0)
+    combined_switch_string = kSwitchPrefixes[0] + combined_switch_string;
   if (!value.empty())
     combined_switch_string += kSwitchValueSeparator + value;
-  argv_.push_back(combined_switch_string);
-  switches_[switch_string] = value;
-#endif
+  // Append the switch and update the switches/arguments divider |begin_args_|.
+  argv_.insert(argv_.begin() + begin_args_++, combined_switch_string);
 }
 
 void CommandLine::AppendSwitchASCII(const std::string& switch_string,
                                     const std::string& value_string) {
-#if defined(OS_WIN)
-  AppendSwitchNative(switch_string, ASCIIToWide(value_string));
-#elif defined(OS_POSIX)
-  AppendSwitchNative(switch_string, value_string);
-#endif
-}
-
-void CommandLine::AppendSwitches(const CommandLine& other) {
-  SwitchMap::const_iterator i;
-  for (i = other.switches_.begin(); i != other.switches_.end(); ++i)
-    AppendSwitchNative(i->first, i->second);
+  AppendSwitchNative(switch_string, Native(value_string));
 }
 
 void CommandLine::CopySwitchesFrom(const CommandLine& source,
                                    const char* const switches[],
                                    size_t count) {
-  for (size_t i = 0; i < count; ++i) {
-    if (source.HasSwitch(switches[i])) {
-      StringType value = source.GetSwitchValueNative(switches[i]);
-      AppendSwitchNative(switches[i], value);
-    }
-  }
+  for (size_t i = 0; i < count; ++i)
+    if (source.HasSwitch(switches[i]))
+      AppendSwitchNative(switches[i], source.GetSwitchValueNative(switches[i]));
+}
+
+CommandLine::StringVector CommandLine::args() const {
+  // Gather all arguments after the last switch (may include kSwitchTerminator).
+  StringVector args(argv_.begin() + begin_args_, argv_.end());
+  // Erase only the first kSwitchTerminator (maybe "--" is a legitimate page?)
+  StringVector::iterator switch_terminator =
+      std::find(args.begin(), args.end(), kSwitchTerminator);
+  if (switch_terminator != args.end())
+    args.erase(switch_terminator);
+  return args;
 }
 
 void CommandLine::AppendArg(const std::string& value) {
@@ -367,117 +362,44 @@ void CommandLine::AppendArgPath(const FilePath& path) {
 }
 
 void CommandLine::AppendArgNative(const CommandLine::StringType& value) {
-#if defined(OS_WIN)
-  command_line_string_.append(L" ");
-  command_line_string_.append(WindowsStyleQuote(value));
-  args_.push_back(value);
-#elif defined(OS_POSIX)
+#if defined(OS_POSIX)
   DCHECK(IsStringUTF8(value));

[Evan Martin, 2011/05/10 23:48:32]

It turns out this code (not your fault) is conceptually wrong -- the "native"
string type on POSIX is bytes, not UTF-8.  So you can safely delete this ifdef
block and the dcheck.

[msw, 2011/05/11 02:28:12]

Done. Should I also nix the OS_WIN IsStringUTF8 check in AppendArg?

-  argv_.push_back(value);
 #endif
-}
-
-void CommandLine::AppendArgs(const CommandLine& other) {
-  if(other.args_.size() <= 0)
-    return;
-#if defined(OS_WIN)
-  command_line_string_.append(L" --");
-#endif  // OS_WIN
-  StringVector::const_iterator i;
-  for (i = other.args_.begin(); i != other.args_.end(); ++i)
-    AppendArgNative(*i);
+  argv_.push_back(value);
 }
 
 void CommandLine::AppendArguments(const CommandLine& other,
                                   bool include_program) {
-#if defined(OS_WIN)
-  // Verify include_program is used correctly.
-  DCHECK(!include_program || !other.GetProgram().empty());
   if (include_program)
-    program_ = other.program_;
-
-  if (!command_line_string_.empty())
-    command_line_string_ += L' ';
-
-  command_line_string_ += other.command_line_string_;
-#elif defined(OS_POSIX)
-  // Verify include_program is used correctly.
-  // Logic could be shorter but this is clearer.
-  DCHECK_EQ(include_program, !other.GetProgram().empty());
-
-  if (include_program)
-    argv_[0] = other.argv_[0];
-
-  // Skip the first arg when copying since it's the program but push all
-  // arguments to our arg vector.
-  for (size_t i = 1; i < other.argv_.size(); ++i)
-    argv_.push_back(other.argv_[i]);
-#endif
-
-  SwitchMap::const_iterator i;
-  for (i = other.switches_.begin(); i != other.switches_.end(); ++i)
-    switches_[i->first] = i->second;
+    SetProgram(other.GetProgram());
+  AppendSwitchesAndArguments(*this, other.argv());
 }
 
 void CommandLine::PrependWrapper(const CommandLine::StringType& wrapper) {
-  // The wrapper may have embedded arguments (like "gdb --args"). In this case,
-  // we don't pretend to do anything fancy, we just split on spaces.
   if (wrapper.empty())
     return;
-  StringVector wrapper_and_args;
-#if defined(OS_WIN)
-  base::SplitString(wrapper, ' ', &wrapper_and_args);
-  program_ = wrapper_and_args[0];
-  command_line_string_ = wrapper + L" " + command_line_string_;
-#elif defined(OS_POSIX)
-  base::SplitString(wrapper, ' ', &wrapper_and_args);
-  argv_.insert(argv_.begin(), wrapper_and_args.begin(), wrapper_and_args.end());
-#endif
+  // The wrapper may have embedded arguments (like "gdb --args"). In this case,
+  // we don't pretend to do anything fancy, we just split on spaces.
+  StringVector wrapper_argv;
+  base::SplitString(wrapper, FILE_PATH_LITERAL(' '), &wrapper_argv);
+  // Prepend the wrapper and update the switches/arguments |begin_args_|.
+  argv_.insert(argv_.begin(), wrapper_argv.begin(), wrapper_argv.end());
+  begin_args_ += wrapper_argv.size();
 }
 
 #if defined(OS_WIN)
 void CommandLine::ParseFromString(const std::wstring& command_line) {
-  TrimWhitespace(command_line, TRIM_ALL, &command_line_string_);
-
-  if (command_line_string_.empty())
+  std::wstring command_line_string;
+  TrimWhitespace(command_line, TRIM_ALL, &command_line_string);
+  if (command_line_string.empty())
     return;
 
   int num_args = 0;
   wchar_t** args = NULL;
+  args = ::CommandLineToArgvW(command_line_string.c_str(), &num_args);
 
-  args = CommandLineToArgvW(command_line_string_.c_str(), &num_args);
-
-  // Populate program_ with the trimmed version of the first arg.
-  TrimWhitespace(args[0], TRIM_ALL, &program_);
-
-  bool parse_switches = true;
-  for (int i = 1; i < num_args; ++i) {
-    std::wstring arg;
-    TrimWhitespace(args[i], TRIM_ALL, &arg);
-
-    if (!parse_switches) {
-      args_.push_back(arg);
-      continue;
-    }
-
-    if (arg == kSwitchTerminator) {
-      parse_switches = false;
-      continue;
-    }
-
-    std::string switch_string;
-    std::wstring switch_value;
-    if (IsSwitch(arg, &switch_string, &switch_value)) {
-      switches_[switch_string] = switch_value;
-    } else {
-      args_.push_back(arg);
-    }
-  }
-
-  if (args)
-    LocalFree(args);
+  CHECK(args);
+  InitFromArgv(num_args, args);
+  LocalFree(args);

[Evan Martin, 2011/05/10 23:48:32]

Nice refactoring!

[msw, 2011/05/11 02:28:12]

:)

 }
 #endif
-
-CommandLine::CommandLine() {
-}