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

content/browser/download/id_type.h

Index: content/browser/download/id_type.h
diff --git a/content/browser/download/id_type.h b/content/browser/download/id_type.h
new file mode 100644
index 0000000000000000000000000000000000000000..abe709c505d027e4f14ae0f45c03cd320b8f57ce
--- /dev/null
+++ b/content/browser/download/id_type.h
@@ -0,0 +1,137 @@
+// 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_BROWSER_DOWNLOAD_ID_TYPE_H_
+#define CONTENT_BROWSER_DOWNLOAD_ID_TYPE_H_

[ncarter (slow), 2016/01/07 00:48:50]

Let's put this in content/common for now.

[Łukasz Anforowicz, 2016/01/07 18:02:13]

Done.

+
+#include <ostream>
+
+#include "base/containers/hash_tables.h"
+
+// Problem:
+//
+//   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::IdType<Foo, uint64_t, 0>;
+//   using BarId = content::IdType<Bar, uint64_t, 0>;

[ncarter (slow), 2016/01/07 00:48:50]

Id prefer if we had a design that didn't require these typedefs: so that it
would be possible to use IdType<Bar> directly, with no other arguments.

Can we accomplish that with defaults, a traits type, or a convention where Bar
declares Bar::kInvalidIdValue?

[Łukasz Anforowicz, 2016/01/07 18:02:12]

I agree that this is desirable, although:

1) I would qualify this further "possible to use IdType<Bar> directly, with no
other arguments, *and no includes or declarations except for id_type.h*".  This
qualification means that "foreign" / unrelated files don't have to include
foo_id.h (or define traits).

2) Just playing devil's advocate - putting all decisions in foo_id.h is better,
because it adheres better to the DRY principle.  For example - if you consider
the proposal below (of IdType32<TypeMarker> convenience alias), then the "32"
duplicates all over the place the decision that could be made once and local to
foo_id.h.  I think this is ok though.
Maybe.  For now I didn't modify kInvalidValue to make it default to 0, because I
think that IdType32 proposal below is sufficient and nicer in practice.  OTOH, I
am open to adding a default for kInvalidValue if you feel strongly about this.
I'd rather not go there:

- Every point of use (i.e. wherever we need full declaration of the type) would
have to see the traits type (to invoke the default contructor one needs to know
the kInvalidValue).  This would mean having to include "foo_id.h" (so why not
just have a type alias inside "foo_id.h") or having to define traits all over
the place (even worse).

- I also think type traits would make things more complex (for users and
maintainers).
Maybe.  This would mean that TypeMarker is more than just a "marker", that it
needs to have its full declaration visible + has to have a specific structure. 
The latter means that one cannot use TypeMarker = types that you cannot change
(OTOH, I guess this can be worked around by using a TypeMarker =
new-artificial-type-that-you-can-change).


Let me propose something that I think is even better.  How about being able to
write something like this (notice the "32" in IdType32<Foo>):

  content::IdType32<Foo> foo_id;
  // or
  using FooId = content::IdType32<Foo>;

We can achieve this with:

  template <typename TypeMarker>
  using IdType32 = IdType<TypeMarker, uint32_t, 0>;
  template <typename TypeMarker>
  using IdType64 = IdType<TypeMarker, uint64_t, 0>;

Potential concerns:
- Naming (Id32 vs IdType32 vs IdTypeUnsigned32 :-)
- Default = 0 is something that we can also argue about... :-/

WDYT?  Let's ship it? :-)

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

I like this idea, a lot.

It looks like, in existing content/ code, signed int32s are far more common than
unsigned int32s for GeId() methods [see link below]. So I think IdType32 should
be signed, and IdTypeU32 sgtm for the unsigned cases.

https://code.google.com/p/chromium/codesearch#search/&q=file:content%20(u)?in...

I think zero is the right default. Routing IDs have a weird null behavior so
they'll have to use the full macro.

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

Done.  (changed IdType32/64 to wrap a signed integer + introduced IdTypeU32/64 +
kept using 0 as the default value here)

+//   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 "content/browser/download/id_type.h"
+//
+//       namespace foo_namespace {
+//
+//       class Foo;
+//       using FooId = content::IdType<Foo, uint64_t, 0>;
+//
+//       }  // 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
+//
+//   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>

[ncarter (slow), 2016/01/07 00:48:50]

I don't really know C++ templates well, so I wonder: if WrappedType is a non
primitive type like std::string, can you actually pass a kInvalidValue here?

[Łukasz Anforowicz, 2016/01/07 18:02:12]

I've tried this ans got an error:
../../content/common/id_type.h:71:66: error: a non-type template parameter
cannot have type 'std::basic_string<char, std::char_traits<char>, std::all
ocator<char> >'

http://en.cppreference.com/w/cpp/language/template_parameters says that the
values has to be of one of the following types:
- std::nullptr_t (since C++11);
- integral type;
- lvalue reference type (to object or to function);
- pointer type (to object or to function);
- pointer to member type (to member object or to member function);
- enumeration type.

I think it is ok to not support strings and similar types *for now*:
1) users of idtype copy
2) implementation of idtype copies
and copying of strings is more expensive than copying of ints.
Additionally, so far all the id types I saw were int-based.

I am not sure if we need to think about support for strings and similar types
*in the future*.  I guess one way to accomplish this would be to hard-code
invalid value to 0.  Another way would be traits route, but this would
complicate usage for users (even in presence of default traits if such thing is
possible).  *But*... I am going to propose to instead limit WrappedType(s) to
integers only (for now and for the long-term future):

- This restriction makes it easy to reason about base::IdType for *users* of the
type (i.e. users can assume that copying is cheap and that the types behave
similar to integers)

- This restriction limits the design space that implementers and maintainers of
base::IdType have to consider.  For example - after restricting WrappedType(s)
to integers only we don't have to worry about WrappedType = std::string OR
WrappedType = <move-only type>.


So - I've added a SFINAE saying that WrappedType has to be an integer.  This is
done via std::is_integral from type_traits [1].  I've tested that this prevents
using non-integer types (I've tested with an enum).  


[1] http://en.cppreference.com/w/cpp/types/is_integral

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

I really like the solution of expose a ready-to-use, single-parameter IdType32 /
IdType64 combo.

There are cases in the code where we might want to wrap a string, but it's best
to leave it as a separate effort, when somebody needs it.

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

Acknowledged.

+struct IdType {

[ncarter (slow), 2016/01/07 00:48:50]

why struct? IdType has no public data members.

[Łukasz Anforowicz, 2016/01/07 18:02:13]

I changed to a class.

RE: why

No particular reason.  My C# background makes me thing struct = stack allocated
value type :-P.  And I based IdType on SurfaceId (although you are right that
this one has a public id field).

+ 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:
+  WrappedType value_;
+};
+
+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_BROWSER_DOWNLOAD_ID_TYPE_H_