Improve map insertion operations.

Add MapInsertOrReplace<>() to insert a key-value pair into a map if the
key is not already present, or replace the existing value for key if the
key is present. The original value can optionally be returned to the
caller in this case.

Map insertions now use either MapInsertOrReplace<>() or
std::map<>::insert() directly.

Use MapInsertOrReplace<>() when the map should be updated to contain a
mapping from a key to a value regardless of whether the key is already
present.

Use std::map<>::insert() to insert a mapping from a key to a value
without replacing any existing mapping from a key, if present. If it is
important to know whether an existing mapping from a key was present,
use the returned std::pair<>.second. If it is important to know the
existing value, use the returned std::pair<>.first->second.

This change has a slight positive impact on performance.

TEST=crashpad_util_test MapInsert.MapInsertOrReplace and others
BUG=
R=scottmg@chromium.org

Review URL: https://codereview.chromium.org/1044273002

handler/mac/crash_report_upload_thread.cc

@@ -19,7 +19,6 @@
 
 #include <map>
 #include <vector>
-#include <utility>
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
@@ -31,6 +30,7 @@
 #include "util/net/http_body.h"
 #include "util/net/http_multipart_builder.h"
 #include "util/net/http_transport.h"
+#include "util/stdlib/map_insert.h"
 
 namespace crashpad {
 
@@ -39,13 +39,10 @@ namespace {
 void InsertOrReplaceMapEntry(std::map<std::string, std::string>* map,
                              const std::string& key,
                              const std::string& value) {
-  auto it = map->find(key);
-  if (it != map->end()) {
+  std::string old_value;
+  if (!MapInsertOrReplace(map, key, value, &old_value)) {
     LOG(WARNING) << "duplicate key " << key << ", discarding value "
-                 << it->second;
-    it->second = value;
-  } else {
-    map->insert(std::make_pair(key, value));
+                 << old_value;
   }
 }
 
@@ -75,11 +72,9 @@ std::map<std::string, std::string> BreakpadHTTPFormParametersFromMinidump(
   std::string list_annotations;
   for (const ModuleSnapshot* module : minidump_process_snapshot.Modules()) {
     for (const auto& kv : module->AnnotationsSimpleMap()) {
-      if (parameters.find(kv.first) != parameters.end()) {
+      if (!parameters.insert(kv).second) {
         LOG(WARNING) << "duplicate key " << kv.first << ", discarding value "
                      << kv.second;
-      } else {
-        parameters.insert(kv);
       }
     }
 

handler/mac/main.cc

@@ -18,7 +18,6 @@
 
 #include <map>
 #include <string>
-#include <utility>
 
 #include "base/files/file_path.h"
 #include "base/logging.h"
@@ -30,6 +29,7 @@
 #include "handler/mac/exception_handler_server.h"
 #include "util/mach/child_port_handshake.h"
 #include "util/posix/close_stdio.h"
+#include "util/stdlib/map_insert.h"
 #include "util/stdlib/string_number_conversion.h"
 #include "util/string/split_string.h"
 #include "util/synchronization/semaphore.h"
@@ -97,13 +97,10 @@ int HandlerMain(int argc, char* argv[]) {
           ToolSupport::UsageHint(me, "--annotation requires KEY=VALUE");
           return EXIT_FAILURE;
         }
-        auto it = options.annotations.find(key);
-        if (it != options.annotations.end()) {
+        std::string old_value;
+        if (!MapInsertOrReplace(&options.annotations, key, value, &old_value)) {
           LOG(WARNING) << "duplicate key " << key << ", discarding value "
-                       << it->second;
-          it->second = value;
-        } else {
-          options.annotations.insert(std::make_pair(key, value));
+                       << old_value;
         }
         break;
       }

minidump/minidump_thread_id_map.cc

@@ -38,11 +38,10 @@ void BuildMinidumpThreadIDMap(
     uint64_t thread_id_64 = thread_snapshot->ThreadID();
     if (thread_id_map->find(thread_id_64) == thread_id_map->end()) {
       uint32_t thread_id_32 = static_cast<uint32_t>(thread_id_64);
-      if (thread_ids_32.find(thread_id_32) != thread_ids_32.end()) {
+      if (!thread_ids_32.insert(thread_id_32).second) {
         collision = true;
         break;
       }
-      thread_ids_32.insert(thread_id_32);
       thread_id_map->insert(std::make_pair(thread_id_64, thread_id_32));
     }
   }

snapshot/mac/mach_o_image_annotations_reader.cc

@@ -17,6 +17,8 @@
 #include <mach-o/loader.h>
 #include <mach/mach.h>
 
+#include <utility>
+
 #include "base/logging.h"
 #include "client/crashpad_info.h"
 #include "client/simple_string_dictionary.h"
@@ -161,10 +163,7 @@ void MachOImageAnnotationsReader::ReadCrashpadSimpleAnnotations(
     if (key_length) {
       std::string key(entry.key, key_length);
       std::string value(entry.value, strnlen(entry.value, sizeof(entry.value)));
-      if (!simple_map_annotations->count(key)) {
-        simple_map_annotations->insert(
-            std::pair<std::string, std::string>(key, value));
-      } else {
+      if (!simple_map_annotations->insert(std::make_pair(key, value)).second) {
         LOG(INFO) << "duplicate simple annotation " << key << " in " << name_;
       }
     }

snapshot/mac/mach_o_image_reader.cc

@@ -20,6 +20,7 @@
 
 #include <limits>
 #include <vector>
+#include <utility>
 
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
@@ -543,15 +544,15 @@ bool MachOImageReader::ReadSegmentCommand(
   // become inconsistent or require cleanup.
 
   const std::string segment_name = segment->Name();
-  const auto& iterator = segment_map_.find(segment_name);
-  if (iterator != segment_map_.end()) {
+  const auto insert_result =
+      segment_map_.insert(std::make_pair(segment_name, segment_index));
+  if (!insert_result.second) {
     LOG(WARNING) << base::StringPrintf("duplicate %s segment at %zu and %zu",
                                        segment_name.c_str(),
-                                       iterator->second,
+                                       insert_result.first->second,
                                        segment_index) << load_command_info;
     return false;
   }
-  segment_map_[segment_name] = segment_index;
 
   mach_vm_size_t vmsize = segment->vmsize();
 

snapshot/mac/mach_o_image_segment_reader.cc

@@ -16,6 +16,8 @@
 
 #include <mach-o/loader.h>
 
+#include <utility>
+
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "snapshot/mac/process_reader.h"
@@ -185,14 +187,14 @@ bool MachOImageSegmentReader::Initialize(ProcessReader* process_reader,
       return false;
     }
 
-    const auto& iterator = section_map_.find(section_name);
-    if (iterator != section_map_.end()) {
+    const auto insert_result =
+        section_map_.insert(std::make_pair(section_name, section_index));
+    if (!insert_result.second) {
       LOG(WARNING) << base::StringPrintf("duplicate section name at %zu",
-                                         iterator->second) << section_info;
+                                         insert_result.first->second)
+                   << section_info;
       return false;
     }
-
-    section_map_[section_name] = section_index;
   }
 
   INITIALIZATION_STATE_SET_VALID(initialized_);

snapshot/mac/mach_o_image_symbol_table_reader.cc

@@ -17,6 +17,8 @@
 #include <mach-o/loader.h>
 #include <mach-o/nlist.h>
 
+#include <utility>
+
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
 #include "util/mac/checked_mach_address_range.h"
@@ -153,15 +155,14 @@ class MachOImageSymbolTableReaderInitializer {
             return false;
           }
 
-          if (external_defined_symbols->count(name)) {
-            LOG(WARNING) << "duplicate symbol " << name << symbol_info;
-            return false;
-          }
-
           MachOImageSymbolTableReader::SymbolInformation this_symbol_info;
           this_symbol_info.value = symbol.n_value;
           this_symbol_info.section = symbol.n_sect;
-          (*external_defined_symbols)[name] = this_symbol_info;
+          if (!external_defined_symbols->insert(
+                  std::make_pair(name, this_symbol_info)).second) {
+            LOG(WARNING) << "duplicate symbol " << name << symbol_info;
+            return false;
+          }
         } else {
           // External indirect symbols may be found in the portion of the symbol
           // table used for external symbols as opposed to indirect symbols when

snapshot/minidump/minidump_simple_string_dictionary_reader.cc

@@ -72,12 +72,10 @@ bool ReadMinidumpSimpleStringDictionary(
       return false;
     }
 
-    if (local_dictionary.find(key) != local_dictionary.end()) {
-      LOG(WARNING) << "duplicate key " << key << ", discarding value " << value;
+    if (!local_dictionary.insert(std::make_pair(key, value)).second) {
+      LOG(ERROR) << "duplicate key " << key;
       return false;
     }
-
-    local_dictionary.insert(std::make_pair(key, value));
   }
 
   dictionary->swap(local_dictionary);

snapshot/minidump/process_snapshot_minidump.cc

@@ -309,16 +309,13 @@ bool ProcessSnapshotMinidump::InitializeModulesCrashpadInfo(
        ++crashpad_module_index) {
     const MinidumpModuleCrashpadInfoLink& minidump_link =
         minidump_links[crashpad_module_index];
-    if (module_crashpad_info_links->find(
-            minidump_link.minidump_module_list_index) !=
-        module_crashpad_info_links->end()) {
+    if (!module_crashpad_info_links
+             ->insert(std::make_pair(minidump_link.minidump_module_list_index,
+                                     minidump_link.location)).second) {
       LOG(WARNING)
           << "duplicate module_crashpad_info_list minidump_module_list_index "
           << minidump_link.minidump_module_list_index;
       return false;
-    } else {
-      module_crashpad_info_links->insert(std::make_pair(
-          minidump_link.minidump_module_list_index, minidump_link.location));
     }
   }
 

tools/mac/run_with_crashpad.cc

@@ -27,6 +27,7 @@
 #include "base/logging.h"
 #include "client/crashpad_client.h"
 #include "tools/tool_support.h"
+#include "util/stdlib/map_insert.h"
 #include "util/string/split_string.h"
 
 namespace crashpad {
@@ -118,13 +119,10 @@ int RunWithCrashpadMain(int argc, char* argv[]) {
           ToolSupport::UsageHint(me, "--annotation requires KEY=VALUE");
           return EXIT_FAILURE;
         }
-        auto it = options.annotations.find(key);
-        if (it != options.annotations.end()) {
+        std::string old_value;
+        if (!MapInsertOrReplace(&options.annotations, key, value, &old_value)) {
           LOG(WARNING) << "duplicate key " << key << ", discarding value "
-                       << it->second;
-          it->second = value;
-        } else {
-          options.annotations.insert(std::make_pair(key, value));
+                       << old_value;
         }
         break;
       }

util/net/http_transport_test.cc

@@ -222,7 +222,7 @@ TEST(HTTPTransport, ValidFormData) {
   builder.SetFormData("key2", "--abcdefg123");
 
   HTTPHeaders headers;
-  headers.insert(builder.GetContentType());
+  EXPECT_TRUE(headers.insert(builder.GetContentType()).second);
 
   HTTPTransportTestFixture test(headers, builder.GetBodyStream(), 200,
       &ValidFormData);

util/stdlib/map_insert.h

@@ -0,0 +1,56 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef CRASHPAD_UTIL_STDLIB_MAP_INSERT_H_
+#define CRASHPAD_UTIL_STDLIB_MAP_INSERT_H_
+
+#include <map>
+#include <utility>
+
+namespace crashpad {
+
+//! \brief Inserts a mapping from \a key to \a value into \a map, or replaces
+//!     an existing mapping so that \a key maps to \value.
+//!
+//! This behaves similarly to `std::map<>::insert_or_assign()` proposed for
+//! C++17, except that the \a old_value parameter is added.
+//!
+//! \param[inout] map The map to operate on.
+//! \param[in] key The key that should be mapped to \a value.
+//! \param[in] value The value that \a key should map to.
+//! \param[out] old_value If \a key was previously present in \a map, this will
+//!     be set to its previous value. This parameter is optional and may be
+//!     `nullptr` if this information is not required.
+//!
+//! \return `false` if \a key was previously present in \a map. If \a old_value
+//!     is not `nullptr`, it will be set to the previous value. `true` if \a
+//!     key was not present in the map and was inserted.
+template <typename T>
+bool MapInsertOrReplace(T* map,
+                        const typename T::key_type& key,
+                        const typename T::mapped_type& value,
+                        typename T::mapped_type* old_value) {
+  const auto result = map->insert(std::make_pair(key, value));
+  if (!result.second) {
+    if (old_value) {
+      *old_value = result.first->second;
+    }
+    result.first->second = value;
+  }
+  return result.second;
+}
+
+}  // namespace crashpad
+
+#endif  // CRASHPAD_UTIL_STDLIB_MAP_INSERT_H_

util/stdlib/map_insert_test.cc

@@ -0,0 +1,54 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/stdlib/map_insert.h"
+
+#include <string>
+
+#include "gtest/gtest.h"
+
+namespace crashpad {
+namespace test {
+namespace {
+
+TEST(MapInsert, MapInsertOrReplace) {
+  std::map<std::string, int> map;
+  int old_value;
+  EXPECT_TRUE(MapInsertOrReplace(&map, "key", 1, &old_value));
+  std::map<std::string, int> expect_map;
+  expect_map["key"] = 1;
+  EXPECT_EQ(expect_map, map);
+
+  EXPECT_FALSE(MapInsertOrReplace(&map, "key", 2, &old_value));
+  EXPECT_EQ(1, old_value);
+  expect_map["key"] = 2;
+  EXPECT_EQ(expect_map, map);
+
+  EXPECT_TRUE(MapInsertOrReplace(&map, "another", 3, &old_value));
+  expect_map["another"] = 3;
+  EXPECT_EQ(expect_map, map);
+
+  // Make sure nullptr is accepted as old_value.
+  EXPECT_TRUE(MapInsertOrReplace(&map, "yet another", 5, nullptr));
+  expect_map["yet another"] = 5;
+  EXPECT_EQ(expect_map, map);
+
+  EXPECT_FALSE(MapInsertOrReplace(&map, "yet another", 6, nullptr));
+  expect_map["yet another"] = 6;
+  EXPECT_EQ(expect_map, map);
+}
+
+}  // namespace
+}  // namespace test
+}  // namespace crashpad

util/util.gyp

@@ -121,6 +121,7 @@
         'posix/symbolic_constants_posix.cc',
         'posix/symbolic_constants_posix.h',
         'stdlib/cxx.h',
+        'stdlib/map_insert.h',
         'stdlib/objc.h',
         'stdlib/pointer_container.h',
         'stdlib/string_number_conversion.cc',
@@ -319,6 +320,7 @@
         'numeric/int128_test.cc',
         'posix/process_info_test.cc',
         'posix/symbolic_constants_posix_test.cc',
+        'stdlib/map_insert_test.cc',
         'stdlib/string_number_conversion_test.cc',
         'stdlib/strlcpy_test.cc',
         'stdlib/strnlen_test.cc',