Reorganize CommandLine code.

base/command_line.cc

Index: base/command_line.cc
diff --git a/base/command_line.cc b/base/command_line.cc
index c0a0f1900563a09a294852324a1d0f38829af30a..b550865df71cc21183c5874b22de003158e0b790 100644
--- a/base/command_line.cc
+++ b/base/command_line.cc
@@ -27,40 +27,38 @@
 
 CommandLine* CommandLine::current_process_commandline_ = NULL;
 
-// Since we use a lazy match, make sure that longer versions (like L"--")
-// are listed before shorter versions (like L"-") of similar prefixes.
+namespace {
+typedef CommandLine::StringType::value_type CharType;
+
+const CharType kSwitchTerminator[] = FILE_PATH_LITERAL("--");
+const 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 wchar_t* const kSwitchPrefixes[] = {L"--", L"-", L"/"};
-const wchar_t kSwitchTerminator[] = L"--";
-const wchar_t kSwitchValueSeparator[] = L"=";
+const CharType* const kSwitchPrefixes[] = {L"--", L"-", L"/"};
 #elif defined(OS_POSIX)
 // Unixes don't use slash as a switch.
-const char* const kSwitchPrefixes[] = {"--", "-"};
-const char kSwitchTerminator[] = "--";
-const char kSwitchValueSeparator[] = "=";
+const CharType* const kSwitchPrefixes[] = {"--", "-"};
 #endif
 
 #if defined(OS_WIN)
-namespace {
-// Lowercase a string.  This is used to lowercase switch names.
-// Is this what we really want?  It seems crazy to me.  I've left it in
-// for backwards compatibility on Windows.
-void Lowercase(std::string* parameter) {
-  transform(parameter->begin(), parameter->end(), parameter->begin(),
-            tolower);
+// Lowercase a string for case-insensitivity of switches.
+// Is this desirable? It exists for backwards compatibility on Windows.
+void Lowercase(std::string* arg) {
+  transform(arg->begin(), arg->end(), arg->begin(), tolower);
 }
 
-// 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, such that CommandLineToArgvW() will always
+// process it as a single argument.
+CommandLine::StringType WindowsStyleQuote(const CommandLine::StringType& arg) {

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

Here's another case where I think the typedef makes it harder to read.  The type
of this function is the same regardless of any compile-time settings: it's a
Windows quote function, so it takes a Windows string.

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

Reverted for now; will become string16 in another change.

   // 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) {
+  if (arg.find_first_of(L" \\\"") == CommandLine::StringType::npos) {
     // No quoting necessary.
     return arg;
   }
 
-  std::wstring out;
+  CommandLine::StringType out;
   out.push_back(L'"');
   for (size_t i = 0; i < arg.size(); ++i) {
     if (arg[i] == '\\') {
@@ -93,94 +91,118 @@ std::wstring WindowsStyleQuote(const std::wstring& arg) {
 
   return out;
 }
-}  // namespace
 #endif
 
-CommandLine::~CommandLine() {
-}
-
-#if defined(OS_WIN)
-CommandLine::CommandLine(NoProgram no_program) {
-}
-
-void CommandLine::ParseFromString(const std::wstring& command_line) {
-  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);
-
-  // 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;
-    }
+// Returns true and fills in |switch_string| and |switch_value| if
+// |parameter_string| represents a switch.
+bool IsSwitch(const CommandLine::StringType& parameter_string,
+              std::string* switch_string,
+              CommandLine::StringType* switch_value) {
+  switch_string->clear();
+  switch_value->clear();
 
-    if (arg == kSwitchTerminator) {
-      parse_switches = false;
+  for (size_t i = 0; i < arraysize(kSwitchPrefixes); ++i) {
+    CommandLine::StringType prefix(kSwitchPrefixes[i]);
+    if (parameter_string.find(prefix) != 0)
       continue;
-    }
 
-    std::string switch_string;
-    std::wstring switch_value;
-    if (IsSwitch(arg, &switch_string, &switch_value)) {
-      switches_[switch_string] = switch_value;
+    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 {
-      args_.push_back(arg);
+      switch_native = parameter_string.substr(
+          switch_start, equals_position - switch_start);
+      *switch_value = parameter_string.substr(equals_position + 1);
     }
+#if defined(OS_WIN)
+    *switch_string = WideToASCII(switch_native);
+    Lowercase(switch_string);
+#else
+    *switch_string = switch_native;
+#endif
+
+    return true;
   }
 
-  if (args)
-    LocalFree(args);
+  return false;
 }
 
-// static
-CommandLine CommandLine::FromString(const std::wstring& command_line) {
-  CommandLine cmd;
-  cmd.ParseFromString(command_line);
-  return cmd;
+}  // 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(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)
-CommandLine::CommandLine(NoProgram no_program) {
-  // Push an empty argument, because we always assume argv_[0] is a program.
-  argv_.push_back("");
+  argv_.push_back(program.value());
+#endif
 }
 
+#if defined(OS_POSIX)
 CommandLine::CommandLine(int argc, const char* const* argv) {
   InitFromArgv(argc, argv);
 }
 
-CommandLine::CommandLine(const std::vector<std::string>& argv) {
+CommandLine::CommandLine(const StringVector& argv) {
   InitFromArgv(argv);
 }
+#endif  // OS_POSIX
 
+// static
+void CommandLine::Init(int argc, const char* const* argv) {
+  delete current_process_commandline_;
+  current_process_commandline_ = new CommandLine;
+#if defined(OS_WIN)
+  current_process_commandline_->ParseFromString(::GetCommandLineW());
+#elif defined(OS_POSIX)
+  current_process_commandline_->InitFromArgv(argc, argv);
+#endif
+}
+
+// static
+void CommandLine::Reset() {
+  DCHECK(current_process_commandline_);
+  delete current_process_commandline_;
+  current_process_commandline_ = NULL;
+}
+
+// static
+CommandLine* CommandLine::ForCurrentProcess() {
+  DCHECK(current_process_commandline_);
+  return current_process_commandline_;
+}
+
+#if defined(OS_WIN)
+// static
+CommandLine CommandLine::FromString(
+    const CommandLine::StringType& command_line) {
+  CommandLine cmd;
+  cmd.ParseFromString(command_line);
+  return cmd;
+}
+#endif  // OS_WIN
+
+#if defined(OS_POSIX)
 void CommandLine::InitFromArgv(int argc, const char* const* argv) {
   for (int i = 0; i < argc; ++i)
     argv_.push_back(argv[i]);
   InitFromArgv(argv_);
 }
 
-void CommandLine::InitFromArgv(const std::vector<std::string>& argv) {
+void CommandLine::InitFromArgv(const StringVector& argv) {
   argv_ = argv;
   bool parse_switches = true;
   for (size_t i = 1; i < argv_.size(); ++i) {
@@ -197,7 +219,7 @@ void CommandLine::InitFromArgv(const std::vector<std::string>& argv) {
     }
 
     std::string switch_string;
-    std::string switch_value;
+    StringType switch_value;
     if (IsSwitch(arg, &switch_string, &switch_value)) {
       switches_[switch_string] = switch_value;
     } else {
@@ -205,72 +227,25 @@ void CommandLine::InitFromArgv(const std::vector<std::string>& argv) {
     }
   }
 }
+#endif  // OS_POSIX
 
-CommandLine::CommandLine(const FilePath& program) {
-  argv_.push_back(program.value());
-}
-
-#endif
-
-// static
-bool CommandLine::IsSwitch(const StringType& parameter_string,
-                           std::string* switch_string,
-                           StringType* switch_value) {
-  switch_string->clear();
-  switch_value->clear();
-
-  for (size_t i = 0; i < arraysize(kSwitchPrefixes); ++i) {
-    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);
-    StringType switch_native;
-    if (equals_position == 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);
-    }
+CommandLine::StringType CommandLine::command_line_string() const {
 #if defined(OS_WIN)
-    *switch_string = WideToASCII(switch_native);
-    Lowercase(switch_string);
-#else
-    *switch_string = switch_native;
+  return command_line_string_;
+#elif defined(OS_POSIX)
+  return JoinString(argv_, ' ');
 #endif
-
-    return true;
-  }
-
-  return false;
 }
 
-// static
-void CommandLine::Init(int argc, const char* const* argv) {
-  delete current_process_commandline_;
-  current_process_commandline_ = new CommandLine;
+FilePath CommandLine::GetProgram() const {
 #if defined(OS_WIN)
-  current_process_commandline_->ParseFromString(::GetCommandLineW());
-#elif defined(OS_POSIX)
-  current_process_commandline_->InitFromArgv(argc, argv);
+  return FilePath(program_);
+#else
+  DCHECK_GT(argv_.size(), 0U);
+  return FilePath(argv_[0]);
 #endif
 }
 
-void CommandLine::Reset() {
-  DCHECK(current_process_commandline_ != NULL);
-  delete current_process_commandline_;
-  current_process_commandline_ = NULL;
-}
-
-// static
-CommandLine* CommandLine::ForCurrentProcess() {
-  DCHECK(current_process_commandline_);
-  return current_process_commandline_;
-}
-
 bool CommandLine::HasSwitch(const std::string& switch_string) const {
   std::string lowercased_switch(switch_string);
 #if defined(OS_WIN)
@@ -305,8 +280,7 @@ CommandLine::StringType CommandLine::GetSwitchValueNative(
   Lowercase(&lowercased_switch);
 #endif
 
-  std::map<std::string, StringType>::const_iterator result =
-      switches_.find(lowercased_switch);
+  SwitchMap::const_iterator result = switches_.find(lowercased_switch);
 
   if (result == switches_.end()) {
     return CommandLine::StringType();
@@ -315,31 +289,30 @@ CommandLine::StringType CommandLine::GetSwitchValueNative(
   }
 }
 
-FilePath CommandLine::GetProgram() const {
-#if defined(OS_WIN)
-  return FilePath(program_);
-#else
-  DCHECK_GT(argv_.size(), 0U);
-  return FilePath(argv_[0]);
-#endif
-}
-
-#if defined(OS_POSIX)
-std::string CommandLine::command_line_string() const {
-  return JoinString(argv_, ' ');
+size_t CommandLine::GetSwitchCount() const {
+  return switches_.size();
 }
-#endif
 
-#if defined(OS_WIN)
 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
+}
+
+void CommandLine::AppendSwitchPath(const std::string& switch_string,
+                                   const FilePath& path) {
+  AppendSwitchNative(switch_string, path.value());
 }
 
 void CommandLine::AppendSwitchNative(const std::string& switch_string,
-                                     const std::wstring& value) {
-  std::wstring combined_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);
@@ -348,26 +321,79 @@ void CommandLine::AppendSwitchNative(const std::string& switch_string,
   command_line_string_.append(combined_switch_string);
 
   switches_[switch_string] = value;
+#elif defined(OS_POSIX)
+  StringType combined_switch_string = kSwitchPrefixes[0] + switch_string;
+  if (!value.empty())
+    combined_switch_string += kSwitchValueSeparator + value;
+  argv_.push_back(combined_switch_string);
+  switches_[switch_string] = value;
+#endif

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

Is the intent that these code paths will become more similar in the future? 
Because otherwise it might be easier to follow if the functions were distinct,
since their bodies are totally different.

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

Yes, the code will become a single cross-platform block in my subsequent change;
I left this alone for now.

 }
 
 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);
+}
+
+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);
+    }
+  }
 }
 
 void CommandLine::AppendArg(const std::string& value) {
+#if defined(OS_WIN)
   DCHECK(IsStringUTF8(value));
   AppendArgNative(UTF8ToWide(value));
+#elif defined(OS_POSIX)
+  AppendArgNative(value);
+#endif
 }
 
-void CommandLine::AppendArgNative(const std::wstring& value) {
+void CommandLine::AppendArgPath(const FilePath& path) {
+  AppendArgNative(path.value());
+}
+
+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)
+  DCHECK(IsStringUTF8(value));
+  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);
 }
 
 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)
@@ -377,54 +403,7 @@ void CommandLine::AppendArguments(const CommandLine& other,
     command_line_string_ += L' ';
 
   command_line_string_ += other.command_line_string_;
-
-  std::map<std::string, StringType>::const_iterator i;
-  for (i = other.switches_.begin(); i != other.switches_.end(); ++i)
-    switches_[i->first] = i->second;
-}
-
-void CommandLine::PrependWrapper(const std::wstring& wrapper) {
-  if (wrapper.empty())
-    return;
-  // 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.
-  std::vector<std::wstring> wrapper_and_args;
-  base::SplitString(wrapper, ' ', &wrapper_and_args);
-  program_ = wrapper_and_args[0];
-  command_line_string_ = wrapper + L" " + command_line_string_;
-}
-
 #elif defined(OS_POSIX)
-void CommandLine::AppendSwitch(const std::string& switch_string) {
-  argv_.push_back(kSwitchPrefixes[0] + switch_string);
-  switches_[switch_string] = "";
-}
-
-void CommandLine::AppendSwitchNative(const std::string& switch_string,
-                                     const std::string& value) {
-  std::string combined_switch_string = kSwitchPrefixes[0] + switch_string;
-  if (!value.empty())
-    combined_switch_string += kSwitchValueSeparator + value;
-  argv_.push_back(combined_switch_string);
-  switches_[switch_string] = value;
-}
-
-void CommandLine::AppendSwitchASCII(const std::string& switch_string,
-                                    const std::string& value_string) {
-  AppendSwitchNative(switch_string, value_string);
-}
-
-void CommandLine::AppendArg(const std::string& value) {
-  AppendArgNative(value);
-}
-
-void CommandLine::AppendArgNative(const std::string& value) {
-  DCHECK(IsStringUTF8(value));
-  argv_.push_back(value);
-}
-
-void CommandLine::AppendArguments(const CommandLine& other,
-                                  bool include_program) {
   // Verify include_program is used correctly.
   // Logic could be shorter but this is clearer.
   DCHECK_EQ(include_program, !other.GetProgram().empty());
@@ -436,66 +415,69 @@ void CommandLine::AppendArguments(const CommandLine& other,
   // arguments to our arg vector.
   for (size_t i = 1; i < other.argv_.size(); ++i)
     argv_.push_back(other.argv_[i]);
+#endif
 
-  std::map<std::string, StringType>::const_iterator i;
+  SwitchMap::const_iterator i;
   for (i = other.switches_.begin(); i != other.switches_.end(); ++i)
     switches_[i->first] = i->second;
 }
 
-void CommandLine::PrependWrapper(const std::string& wrapper) {
+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.
-  std::vector<std::string> wrapper_and_args;
+  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
 }
 
-#endif
+#if defined(OS_WIN)
+void CommandLine::ParseFromString(const CommandLine::StringType& command_line) {
+  TrimWhitespace(command_line, TRIM_ALL, &command_line_string_);
 
-void CommandLine::AppendSwitchPath(const std::string& switch_string,
-                                   const FilePath& path) {
-  AppendSwitchNative(switch_string, path.value());
-}
+  if (command_line_string_.empty())
+    return;
 
-void CommandLine::AppendSwitches(const CommandLine& other) {
-  std::map<std::string, StringType>::const_iterator i;
-  for (i = other.switches_.begin(); i != other.switches_.end(); ++i)
-    AppendSwitchNative(i->first, i->second);
-}
+  int num_args = 0;
+  wchar_t** args = NULL;
 
-void CommandLine::AppendArgPath(const FilePath& path) {
-  AppendArgNative(path.value());
-}
+  args = CommandLineToArgvW(command_line_string_.c_str(), &num_args);
 
-void CommandLine::AppendArgs(const CommandLine& other) {
-  if(other.args_.size() <= 0)
-    return;
+  // Populate program_ with the trimmed version of the first arg.
+  TrimWhitespace(args[0], TRIM_ALL, &program_);
 
-#if defined(OS_WIN)
-  command_line_string_.append(L" --");
-#endif  // OS_WIN
-  std::vector<StringType>::const_iterator i;
-  for (i = other.args_.begin(); i != other.args_.end(); ++i)
-    AppendArgNative(*i);
-}
+  bool parse_switches = true;
+  for (int i = 1; i < num_args; ++i) {
+    StringType arg;
+    TrimWhitespace(args[i], TRIM_ALL, &arg);
 
-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);
+    if (!parse_switches) {
+      args_.push_back(arg);
+      continue;
     }
-  }
-}
 
-// private
-CommandLine::CommandLine() {
-}
+    if (arg == kSwitchTerminator) {
+      parse_switches = false;
+      continue;
+    }
 
-// static
-CommandLine* CommandLine::ForCurrentProcessMutable() {
-  DCHECK(current_process_commandline_);
-  return current_process_commandline_;
+    std::string switch_string;
+    StringType switch_value;
+    if (IsSwitch(arg, &switch_string, &switch_value)) {
+      switches_[switch_string] = switch_value;
+    } else {
+      args_.push_back(arg);
+    }
+  }
+
+  if (args)
+    LocalFree(args);
 }
+#endif