Resolve signed-unsigned comparison issues in TypeSafeIndex

[SeanCurtis-TRI]

When comparing an unsigned integral value with a TypeSafeIndex, we needed to account for the possibility that the unsigned integral value could contain larger values than the TypeSafeIndex. This was done by comparing the unsigned integer with the maximum int (the largest type safe
index value). This is intrinsically a signed-unsigned comparision which is problematic.

Instead, we cast the maximum int to the unsigned integral type. If the unsigned type is larger (more bits), the relationship will be fully preserved and unsigned values that are larger than can be represented will be detected.

If the unsigned type has fewer bits, the maximum int (01111.1111) will be truncated to a number consisting of all 1s, the maximum unsigned value of that number of bits. We are guaranteed that the unsigned value must be less than or equal to that truncated value which means its less than or
equal to the full maximum index.

In other words:

value <= static_cast(kMaxIndex) is always true if
sizeof(U) < sizeof(int).

resolves #13482

---

This change is 

[SeanCurtis-TRI]

@drake-jenkins-bot linux-focal-unprovisioned-gcc-bazel-experimental-debug please

[jamiesnape]

Lint is unhappy, but looks like the Focal build is going to be good otherwise.

[SeanCurtis-TRI]

+@jamiesnape for feature review.

I've fixed the lint. I"ll defer to you if you want to see the focal CI run again.

Reviewable status: LGTM missing from assignee jamiesnape, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jamiesnape)

[jamiesnape]

I don't think we need to re-run Focal CI.

Reviewed 2 of 2 files at r1.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

[SeanCurtis-TRI]

+@jwnimmer-tri for platform review, please.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri)

[jwnimmer-tri]

Reviewed 2 of 2 files at r1.
Reviewable status: 4 unresolved discussions (waiting on @SeanCurtis-TRI)

---

common/type_safe_index.h, line 486 at r1 (raw file):

  // Invocations provide a string explaining the origin of the bad value.
  static void AssertValid(int64_t index, const char* source) {
    if (index < 0 || index > std::numeric_limits<int>::max()) {

nit kMaxIndex?

---

common/type_safe_index.h, line 497 at r1 (raw file):

  // the current index value.
  void AssertNoOverflow(int delta, const char* source) const {
    if (delta > 0 && index_ > std::numeric_limits<int>::max() - delta) {

nit kMaxIndex?

---

common/type_safe_index.h, line 513 at r1 (raw file):

  int index_{kDefaultInvalid};

  // The largest representable index.

The casts of this constant to a smaller unsigned type (like uint16_t) earlier in this file are only valid if the maximum here is filled with 1's bits. So if someone changed this to be 1 billion as a round number, instead of 2^32-1, it would be very bad. How about a cautionary or explanatory comment to that effect here?

---

common/test/type_safe_index_test.cc, line 567 at r1 (raw file):

  EXPECT_FALSE(equal_index > big_unsigned);
  EXPECT_TRUE(equal_index >= big_unsigned);
}

nit It hurt my brain that the first new test case had "equal" stanza in the middle, but this case had "equal" stanza last.

[SeanCurtis-TRI]

Comments addressed.

Reviewable status: 1 unresolved discussion (waiting on @jamiesnape and @jwnimmer-tri)

---

common/type_safe_index.h, line 486 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit kMaxIndex?

Done

---

common/type_safe_index.h, line 497 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit kMaxIndex?

Done

---

common/type_safe_index.h, line 513 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The casts of this constant to a smaller unsigned type (like uint16_t) earlier in this file are only valid if the maximum here is filled with 1's bits. So if someone changed this to be 1 billion as a round number, instead of 2^32-1, it would be very bad. How about a cautionary or explanatory comment to that effect here?

Done

---

common/test/type_safe_index_test.cc, line 567 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit It hurt my brain that the first new test case had "equal" stanza in the middle, but this case had "equal" stanza last.

Done

[jwnimmer-tri]

Reviewed 2 of 2 files at r2.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),jamiesnape