ABANDONED CL: Adding a compile-time safe base::IdType<...> into //base.

ABANDONED CL: This CL is replaced by crrev.com/1548443002

------------------------

Adding a compile-time safe base::IdType<...> into //base.

base::IdType is a generalization of cc::SurfaceId.

base::IdType provides a type that for most practical purposes behaves
like an integer, but provides extra compile-time safety, by disallowing
implicit coercions from raw integers and/or from other, unrelated
base::IdTypes.  For more information see the comments in base/id_type.h.

Implementation of SurfaceId is being converted
to reuse base::IdType<...> in crrev.com/1496103002.

save_package_id is being converted from a raw integer
to base::IdType<...> in crrev.com/1492283004.

BUG=565545
.

[Łukasz Anforowicz, 2015-12-03 20:52:11]

Description was changed from

==========
Adding a compile-time safe base::IdType<...> into //base.

base::IdType provides a type that for most practical purposes behaves
like an integer, but provides extra compile-time safety, by disallowing
implicit coercions from raw integers and/or from other, unrelated
base::IdTypes.  For more information see the comments in base/id_type.h.

Implementation of SurfaceId is being converted
to reuse base::IdType<...> in crrev.com/TODO.

save_package_id is being converted from a raw integer
to base::IdType<...> in crrev.com/TODO.

BUG=565545.
==========

to

==========
Adding a compile-time safe base::IdType<...> into //base.

base::IdType provides a type that for most practical purposes behaves
like an integer, but provides extra compile-time safety, by disallowing
implicit coercions from raw integers and/or from other, unrelated
base::IdTypes.  For more information see the comments in base/id_type.h.

Implementation of SurfaceId is being converted
to reuse base::IdType<...> in crrev.com/1496103002.

save_package_id is being converted from a raw integer
to base::IdType<...> in crrev.com/1492283004.

BUG=565545.
==========

[Łukasz Anforowicz, 2015-12-03 20:53:12]

Description was changed from

==========
Adding a compile-time safe base::IdType<...> into //base.

base::IdType provides a type that for most practical purposes behaves
like an integer, but provides extra compile-time safety, by disallowing
implicit coercions from raw integers and/or from other, unrelated
base::IdTypes.  For more information see the comments in base/id_type.h.

Implementation of SurfaceId is being converted
to reuse base::IdType<...> in crrev.com/1496103002.

save_package_id is being converted from a raw integer
to base::IdType<...> in crrev.com/1492283004.

BUG=565545.
==========

to

==========
Adding a compile-time safe base::IdType<...> into //base.

base::IdType is a generalization of cc::SurfaceId.

base::IdType provides a type that for most practical purposes behaves
like an integer, but provides extra compile-time safety, by disallowing
implicit coercions from raw integers and/or from other, unrelated
base::IdTypes.  For more information see the comments in base/id_type.h.

Implementation of SurfaceId is being converted
to reuse base::IdType<...> in crrev.com/1496103002.

save_package_id is being converted from a raw integer
to base::IdType<...> in crrev.com/1492283004.

BUG=565545.
==========

[Łukasz Anforowicz, 2015-12-04 22:04:41]

lukasza@chromium.org changed reviewers:
+ danakj@chromium.org

[Łukasz Anforowicz, 2015-12-04 22:04:41]

danakj@, could you please take a look?

(full disclosure - the CL under review has been carved out from a bigger draft
CL that has been abandoned at crrev.com/1447363004 and that got some preliminary
feedback from piman@, jam@ and dcheng@;  at this point I think I've incorporated
all of their feedback and I am ready for an owners review;  I am asking for
review from you mainly because I see that you are an owner of base [unlike them]
and also an owner of cc [so you also look like a good reviewer for one of the
follow-up CLs - crrev.com/1496103002]).

[danakj, 2015-12-07 19:24:49]

I'm not sold that this is sometihng we need in base.

For the vast majority of of ids in chromium, a typedef is more than enough. It's
nice to have a type name other than int, but the extra type-safety is kinda
overkill.

For some ids (I can think of only one), that we plumb around through a lot of
layers of code, I've appreciated type-safe semantics. I don't know why it's not
enough to just make a struct for those specific Ids, like:

struct MyId {
  int id;
};

[Łukasz Anforowicz, 2015-12-08 18:07:08]

Let me try to enumerate the possible requirements (I guess all of them are
negotiable):

R1: Passing foo_id as an argument of a function expecting bar_id shouldn't
compile.

R2: Passing foo_id as a key of a map expecting bar_id shouldn't compile.

R3: Parameter declaration should say FooId rather than int.

R4: Map declaration should say that FooId is a key (rather than int).

R5: Low mental overhead when reading / writing id related code.

R3 and R4 are readability improvement.
R1 and R2 improve confidence in code correctness (i.e. when refactoring).
None of the requirements are critical for code health, but I think that they are
definitely desirable
and the main question is how to achieve them with low overhead (readability
overhead / maintenance overhead / perf overhead).

The typedef suggestion addresses R3 and R4 but fails to address R1 and R2.
R5 is ok from simplicity perspective (it is just a simple int), but is not so
good when having
to read/write functions taking multiple ids (i.e. have to be careful to pass
them in the right order).

struct FooId { int id; } addresses R1 and R3, but fails to address R2 and R4:
- If I keep struct definition small, then I have to declare std::map<int, ...>
  (not meeting R2 and R4) and use id.id when interacting with the map (making R5
not so good).
- Defining == and either < or std::hash helps with R2 and R4, but makes the code
more
  complicated (again making R5 not so good).

On 2015/12/07 19:24:49, danakj (behind on reviews) wrote:
> I'm not sold that this is sometihng we need in base.
> 
> For the vast majority of of ids in chromium, a typedef is more than enough.
It's
> nice to have a type name other than int, but the extra type-safety is kinda
> overkill.

Could you help me understand why you think that this is an "overkill"?
Do you think that type safety is not that desirable here?
Or that the cost of getting type safety is too high?

> For some ids (I can think of only one), that we plumb around through a lot of
> layers of code, I've appreciated type-safe semantics. I don't know why it's
not
> enough to just make a struct for those specific Ids, like:
> 
> struct MyId {
>   int id;
> };

I think that once one has to define == and < or std::hash, the cost of
individual MyId structs is higher than the cost of base::IdType<...>.
Also - at the top of my reply I try to explain why the struct doesn't quite meet
the requirements (well, it kind of does, but not quite :-).

I would really like to be able to use a type-safe SavePackageId and SaveItemId
under content/browser/downloads.  I am hesitant to introduce 2 structs and
define
equality and std::hash for both of them - this would feel like quite a bit of
duplicated
code (and would be costly from the perspective of having to maintain the same
code
in multiple places).

[Łukasz Anforowicz, 2015-12-18 20:46:51]

Description was changed from

==========
Adding a compile-time safe base::IdType<...> into //base.

base::IdType is a generalization of cc::SurfaceId.

base::IdType provides a type that for most practical purposes behaves
like an integer, but provides extra compile-time safety, by disallowing
implicit coercions from raw integers and/or from other, unrelated
base::IdTypes.  For more information see the comments in base/id_type.h.

Implementation of SurfaceId is being converted
to reuse base::IdType<...> in crrev.com/1496103002.

save_package_id is being converted from a raw integer
to base::IdType<...> in crrev.com/1492283004.

BUG=565545.
==========

to

==========
POTENTIALLY ABANDONED CL: This CL will probably get replaced by
crrev.com/1529363006

------------------------

Adding a compile-time safe base::IdType<...> into //base.

base::IdType is a generalization of cc::SurfaceId.

base::IdType provides a type that for most practical purposes behaves
like an integer, but provides extra compile-time safety, by disallowing
implicit coercions from raw integers and/or from other, unrelated
base::IdTypes.  For more information see the comments in base/id_type.h.

Implementation of SurfaceId is being converted
to reuse base::IdType<...> in crrev.com/1496103002.

save_package_id is being converted from a raw integer
to base::IdType<...> in crrev.com/1492283004.

BUG=565545.
==========

[Łukasz Anforowicz, 2016-02-11 00:04:53]

Description was changed from

==========
POTENTIALLY ABANDONED CL: This CL will probably get replaced by
crrev.com/1529363006

------------------------

Adding a compile-time safe base::IdType<...> into //base.

base::IdType is a generalization of cc::SurfaceId.

base::IdType provides a type that for most practical purposes behaves
like an integer, but provides extra compile-time safety, by disallowing
implicit coercions from raw integers and/or from other, unrelated
base::IdTypes.  For more information see the comments in base/id_type.h.

Implementation of SurfaceId is being converted
to reuse base::IdType<...> in crrev.com/1496103002.

save_package_id is being converted from a raw integer
to base::IdType<...> in crrev.com/1492283004.

BUG=565545.
==========

to

==========
ABANDONED CL: This CL is replaced by crrev.com/1548443002

------------------------

Adding a compile-time safe base::IdType<...> into //base.

base::IdType is a generalization of cc::SurfaceId.

base::IdType provides a type that for most practical purposes behaves
like an integer, but provides extra compile-time safety, by disallowing
implicit coercions from raw integers and/or from other, unrelated
base::IdTypes.  For more information see the comments in base/id_type.h.

Implementation of SurfaceId is being converted
to reuse base::IdType<...> in crrev.com/1496103002.

save_package_id is being converted from a raw integer
to base::IdType<...> in crrev.com/1492283004.

BUG=565545.
==========