CommandLine refactoring and cleanup.

base/command_line.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/command_line.h"
 
 #include <algorithm>
 #include <ostream>
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/string_split.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <shellapi.h>
 #endif
 
 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)
-// 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);
+  return ASCIIToWide(string);
+#elif defined(OS_POSIX)
+  return string;
+#endif
 }
 
-// Quote a string if necessary, such that CommandLineToArgvW() will always
-// process it as a single argument.
-std::wstring WindowsStyleQuote(const std::wstring& arg) {
+CommandLine::StringType Native(const std::wstring& string) {
+#if defined(OS_WIN)
+  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 *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) {
     // No quoting necessary.
     return arg;
   }
 
   std::wstring out;
   out.push_back(L'"');
   for (size_t i = 0; i < arg.size(); ++i) {
@@ skipping 19 matching lines @@
     } else if (arg[i] == '"') {
       out.push_back('\\');
       out.push_back('"');
     } else {
       out.push_back(arg[i]);
     }
   }
   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
 }
-#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 equals_position = string.find(kSwitchValueSeparator);
+  CommandLine::StringType switch_native = string.substr(0, equals_position);
+#if defined(OS_WIN)
+  *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;
+}
 
-    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);
-    }
-#if defined(OS_WIN)
-    *switch_string = WideToASCII(switch_native);
-    Lowercase(switch_string);
-#else
-    *switch_string = switch_native;
-#endif
+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 true;
+    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);
   }
-
-  return false;
 }
 
 }  // 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() {
 }
 
 // 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)
   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 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;
+    CommandLine::StringType arg = argv_[i];
+    std::string switch_string;
+    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));
     }
-
-    if (arg == kSwitchTerminator) {
-      parse_switches = false;
-      continue;
-    }
-
-    std::string switch_string;
-    StringType switch_value;
-    if (IsSwitch(arg, &switch_string, &switch_value)) {
-      switches_[switch_string] = switch_value;
-    } else {
-      args_.push_back(arg);
+    else {
+      string.append(Quote(arg));
     }
   }
-}
-#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
+  return string;
 }
 
 FilePath CommandLine::GetProgram() const {
-#if defined(OS_WIN)
-  return FilePath(program_);
-#else
-  DCHECK_GT(argv_.size(), 0U);
-  return FilePath(argv_[0]);
-#endif
+  return FilePath(program_string_);
+}
+
+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 "";
   }
 #if defined(OS_WIN)
   return WideToASCII(value);
 #else
   return value;
 #endif
 }
 
 FilePath CommandLine::GetSwitchValuePath(
     const std::string& switch_string) const {
   return FilePath(GetSwitchValueNative(switch_string));
 }
 
 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 {
   return switches_.size();
 }
 
 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,
                                    const FilePath& path) {
   AppendSwitchNative(switch_string, path.value());
 }
 
 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) {
 #if defined(OS_WIN)
   DCHECK(IsStringUTF8(value));
   AppendArgNative(UTF8ToWide(value));
 #elif defined(OS_POSIX)
   AppendArgNative(value);
 #endif
 }
 
 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)
+#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?

+#endif
   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)
-    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) {
+  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.
-  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
+  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() {
-}