Introducing SavePackageId and SaveItemId as distinct IdType<...>-based types.

content/common/id_type.h

Index: content/common/id_type.h
diff --git a/content/common/id_type.h b/content/common/id_type.h
new file mode 100644
index 0000000000000000000000000000000000000000..e13081a3b2fef5820385253a442296ede71c9db1
--- /dev/null
+++ b/content/common/id_type.h
@@ -0,0 +1,158 @@
+// Copyright 2015 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.
+
+#ifndef CONTENT_COMMON_ID_TYPE_H_
+#define CONTENT_COMMON_ID_TYPE_H_
+
+#include <stdint.h>
+#include <ostream>
+#include <type_traits>
+
+#include "base/containers/hash_tables.h"
+
+// Problem:

[ncarter (slow), 2016/01/07 20:14:32]

For templates like this, the definitions that users care about often wind up
buried in the middle of the file, so reading the code top-down is pretty hard.
So the comment needs to do a good job of summarizing the useful parts.

I find the "Problem:/Solution:" intro a little terse, since it kind of requires
you to spot the swapped-parameters bug. If such bugs were easy to spot, we
wouldn't need IdType! Here's my attempt at an intro:

// IdType32<>, IdType64<>, IdType<>: wrap an integer id in a custom type.
//
// IdType32<Foo> is an alternative to int, for a class Foo with methods like:
//
//    int GetId() { return id_; };
//    static Foo* FromId(int id) { return g_all_foos_by_id[id]; }
//
// Such methods are a standard means of safely referring to objects across
// thread and process boundaries. But if a nearby classes Bar also represents
// its IDs as a bare int, horrific mixups are possible -- one example, of many,
// is http://crrev.com/365437. IdType<> offers compile-time protection against
// such mishaps, since IdType32<Foo> is incompatible with IdType32<Bar>, even
// though both just compile down to an int32_t.
//
// Templates in this file:
//   IdType32<T> / IdTypeU32<T>: Signed / unsigned 32-bit IDs
//   IdType64<T> / IdTypeU64<T>: Signed / unsigned 64-bit IDs
//   IdType<>: For when you need a different underlying type

[Łukasz Anforowicz, 2016/01/07 21:43:01]

Good point.  Done.  OTOH I tweaked your suggestion slightly, so please take one
more brief look.

PS. Does one say "are a standard means" or "are standard means"?  I am getting
confused here as a non-native speaker :-/

PPS. I see that you are a one-space-after-a-period-person :-)  Other comments in
id_type.h were typed by me (a two-spaces-after-a-period-person).  I guess I need
to pick one convention and stick to it.

+//
+//   void f(int foo_id, int bar_id);
+//   ...
+//   // Arguments passed in a wrong order:
+//   f(bar_id, foo_id);  // Compiles - sigh... :-(
+//
+// Solution:
+//
+//   using FooId = content::IdType32<Foo>;
+//   using BarId = content::IdType32<Bar>;
+//   void f(FooId foo_id, BarId bar_id);
+//   ...
+//   // Arguments passed in a wrong order:
+//   f(bar_id, foo_id);  // Doesn't compile anymore - yay! :-)
+//
+// Typically FooId would be declared like this:
+//
+//   foo_id.h:
+//
+//       #include <stdint.h>
+//       #include "content/common/id_type.h"
+//
+//       namespace foo_namespace {
+//
+//       class Foo;
+//       using FooId = content::IdType<Foo, uint64_t, 0>;
+//
+//       // A type alias provided by id_type.h can also be used:
+//       // using FooId = content::IdType64<Foo>;
+//
+//       }  // namespace foo_namespace
+//
+// For most practical purposes, FooId declared like above behaves just like
+// an uint64_t, except that:
+//
+//   1. FooId only supports a subset of uint64_t operations:
+//      - ==, !=, <, hashing
+//      - stream output
+//      - copy construction and assignment

[ncarter (slow), 2016/01/07 20:14:32]

Would it be better to talk about what the supported operations let you do?

"All IdType<> variants support comparison and hashing, so you can use them as
arguments to ASSERT_EQ or as keys in STL containers."

[Łukasz Anforowicz, 2016/01/07 21:43:01]

Done.

I've added a paragraph following your suggestion (and also mentioning memcpy
[which I think is relatively important], restraining myself from mentioning
other random operations [like stream output or ipc pickling], and finally
mentioning that some things like addition or multiplication are not supported).

In the spirit of simplicity and brevity I've also yanked out the "Typically
FooId would be declared like this" and I've moved item 4 to id_type_unittest.cc.

+//
+//   2. FooId does not implicitly coerce to/from other uint64_t values and
+//      to/from content::IdType<SomeOtherType, ...>.  Explicit coercion to/from
+//      uint64_t is possible through the following FooId methods:
+//      - static FooId FromUnsafeValue(uint64_t)
+//      - uint64_t GetUnsafeValue()
+//
+//   3. Default-constructed FooId contains a "null" / "invalid" value specified
+//      by the 3rd argument of the IdType<...> template.  Testing against this
+//      value can be done by the following FooId methods:
+//      - bool is_valid()
+//      - bool is_null()
+//
+//   4. The presence of a custom default constructor means that FooId is not a
+//      "trivial" class and therefore is not a POD type (unlike an uint64_t).
+//      At the same time FooId has almost all of the properties of a POD type:
+//      - is "trivially copyable" (i.e. is memcpy-able),
+//      - has "standard layout" (i.e. interops with things expecting C layout).
+//      See http://stackoverflow.com/a/7189821 for more info about these
+//      concepts.
+
+namespace content {
+
+template <typename TypeMarker, typename WrappedType, WrappedType kInvalidValue>
+class IdType {
+ public:
+  IdType() : value_(kInvalidValue) {}
+  bool is_valid() const { return value_ != kInvalidValue; }
+  bool is_null() const { return value_ == kInvalidValue; }
+
+  static IdType FromUnsafeValue(WrappedType value) { return IdType(value); }
+  WrappedType GetUnsafeValue() const { return value_; }
+
+  IdType(const IdType& other) = default;
+  IdType& operator=(const IdType& other) = default;
+
+  bool operator==(const IdType& other) const { return value_ == other.value_; }
+  bool operator!=(const IdType& other) const { return value_ != other.value_; }
+  bool operator<(const IdType& other) const { return value_ < other.value_; }
+
+ protected:
+  explicit IdType(WrappedType val) : value_(val) {}
+
+ private:
+  // In theory WrappedType could be any type that supports ==, <, <<, std::hash,
+  // etc., but to make things simpler (both for users and for maintainers) we
+  // explicitly restrict the design space to integers.  This means the users
+  // can safely assume that IdType is relatively small and cheap to copy
+  // and the maintainers don't have to worry about WrappedType being a complex
+  // type (i.e. std::string or std::pair or a move-only type).
+  using IntegralWrappedType =
+      typename std::enable_if<std::is_integral<WrappedType>::value,
+                              WrappedType>::type;
+  IntegralWrappedType value_;
+};
+
+// Type aliases for convenience:
+template <typename TypeMarker>
+using IdType32 = IdType<TypeMarker, uint32_t, 0>;
+template <typename TypeMarker>
+using IdType64 = IdType<TypeMarker, uint64_t, 0>;
+
+template <typename TypeMarker, typename WrappedType, WrappedType kInvalidValue>
+std::ostream& operator<<(
+    std::ostream& stream,
+    const IdType<TypeMarker, WrappedType, kInvalidValue>& id) {
+  return stream << id.GetUnsafeValue();
+}
+
+}  // namespace content
+
+namespace BASE_HASH_NAMESPACE {
+
+template <typename TypeMarker, typename WrappedType, WrappedType kInvalidValue>
+struct hash<content::IdType<TypeMarker, WrappedType, kInvalidValue>> {
+  using argument_type = content::IdType<TypeMarker, WrappedType, kInvalidValue>;
+  using result_type = std::size_t;
+  result_type operator()(const argument_type& id) const {
+    return BASE_HASH_NAMESPACE::hash<WrappedType>()(id.GetUnsafeValue());
+  }
+};
+
+}  // namespace BASE_HASH_NAMESPACE
+
+// If defined(COMPILER_MSVC) then BASE_HASH_NAMESPACE == std.
+// In this case we need to avoid defininig std::hash<...> second time
+// (it has already been defined above).
+#if !defined(COMPILER_MSVC)
+
+namespace std {
+
+template <typename TypeMarker, typename WrappedType, WrappedType kInvalidValue>
+struct hash<content::IdType<TypeMarker, WrappedType, kInvalidValue>> {
+  using argument_type = content::IdType<TypeMarker, WrappedType, kInvalidValue>;
+  using result_type = std::size_t;
+  result_type operator()(const argument_type& id) const {
+    return std::hash<WrappedType>()(id.GetUnsafeValue());
+  }
+};
+
+}  // namespace std;
+
+#endif  // !defined(COMPILER_MSVC)
+
+#endif  // CONTENT_COMMON_ID_TYPE_H_