Take a StringPiece when looking up CommandLine switches.

base/command_line.h

Index: base/command_line.h
diff --git a/base/command_line.h b/base/command_line.h
index 19df40c4c478bfa9befe59ad9eefc63f5b33c55f..e6f999536e143721d5c31a92ebf487bf7133c825 100644
--- a/base/command_line.h
+++ b/base/command_line.h
@@ -22,6 +22,7 @@
 
 #include "base/base_export.h"
 #include "base/strings/string16.h"
+#include "base/strings/string_piece.h"
 #include "build/build_config.h"
 
 namespace base {
@@ -52,6 +53,10 @@ class BASE_EXPORT CommandLine {
   CommandLine(int argc, const CharType* const* argv);
   explicit CommandLine(const StringVector& argv);
 
+  // Override copy and assign to ensure |switches_by_stringpiece_| is valid.
+  CommandLine(const CommandLine& other);

[tapted, 2015/04/15 00:07:53]

it's tempting to make this explicit... I bet a lot of calls are unintentionally
copying this by value when they can just take a reference instead. That's for a
follow-up in any case.

+  CommandLine& operator=(const CommandLine& other);
+
   ~CommandLine();
 
 #if defined(OS_WIN)
@@ -142,19 +147,19 @@ class BASE_EXPORT CommandLine {
   void SetProgram(const FilePath& program);
 
   // Returns true if this command line contains the given switch.
-  // (Switch names are case-insensitive).
+  // Switch names should only be lowercase.

[tapted, 2015/04/15 00:07:53]

nit: should only -> must

[jackhou1, 2015/04/17 00:56:10]

Done.

   // The second override provides an optimized version to avoid inlining the
   // codegen for the string allocation.

[tapted, 2015/04/15 00:07:53]

This last sentence in the comment is no longer valid. For a StringPiece, there
is still codegen for the strlen and the stack allocation of the StringPiece,
versus just passing a pointer to the char* in a register. I'd expect a ~24kB hit
to sizes by removing the overload.

I'd suggest keeping the overload, but updating this comment to say something
like "... to avoid inlining the codegen to find the length of the constant and
construct a StringPiece."

[jackhou1, 2015/04/17 00:56:10]

Done.

-  bool HasSwitch(const std::string& switch_string) const;
-  bool HasSwitch(const char switch_constant[]) const;
+  bool HasSwitch(const base::StringPiece& 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;
+  // Switch names should only be lowercase.
+  std::string GetSwitchValueASCII(const base::StringPiece& switch_string) const;
+  FilePath GetSwitchValuePath(const base::StringPiece& switch_string) const;
+  StringType GetSwitchValueNative(const base::StringPiece& switch_string) const;
 
-  // Get a copy of all switches, along with their values.
+  // Get a reference to the internal switch map.
   const SwitchMap& GetSwitches() const { return switches_; }
 
   // Append a switch [with optional value] to the command line.
@@ -223,6 +228,10 @@ class BASE_EXPORT CommandLine {
   // Parsed-out switch keys and values.
   SwitchMap switches_;
 
+  // A mirror of |switches_| with only references to the actual strings.
+  // Used for allocation-free lookups.

[tapted, 2015/04/15 00:07:53]

nit: mention mapped_type points to an object in |switches_|

[jackhou1, 2015/04/17 00:56:10]

Done.

+  std::map<base::StringPiece, const StringType*> switches_by_stringpiece_;

[tapted, 2015/04/15 00:07:53]

It might be less confusing to just store a value for the mapped type - it would
be good to have numbers on the benefit of avoiding the extra copies

[jackhou1, 2015/04/17 00:56:10]

I'm think the memory/performance impact would be negligible since CommandLines
typically have very few switches. But I also think the impact on code clarity of
storing a pointer is not that big. Since the key is basically a pointer into
|switches| it seems natural that the mapped_type is as well.

But I don't mind either way if you feel strongly about it.

+
   // The index after the program and switches, any arguments start here.
   size_t begin_args_;
 };