Consistently DCHECK the index >= 0 in value store getters

[danakj]

Instead of DCHECKing inside GetRawIndex sometimes and in the Get/Add methods other times, consistently DCHECK as far up the call stack as possible by deferring the DCHECK to the Get/Add methods, through a new TryGetRawIndex that passes through untagged id indices such as None.

Keep the GetRawIndex method around with the DCHECK for other callers, as we often call that and then index into some other array with it, and the DCHECK makes good sense there for those callers.

Make the IdTag relationship between RelationalValueStore and ValueStore more clear. They have the same ID values, as they have the same indices and the same IdTag.

[danakj]

Based on #6308, the first commit is get-raw-index

[jonmeow]

Instead of DCHECKing inside GetRawIndex sometimes and in the Get/Add methods other times, consistently DCHECK as far up the call stack as possible by deferring the DCHECK to the Get/Add methods, through a new TryGetRawIndex that passes through untagged id indices such as None.

It looks like, before this change, GetRawIndex would always CHECK-fail on the index because every implementation deferred to ValueStore::GetRawIndex, and that implementation contained a CHECK-fail. In other words, maybe there was no reason to have callers add a CHECK-fail because it was centrally implemented.

Am I misunderstanding something? @dwblaikie do you have thoughts on this change?

[danakj]

Instead of DCHECKing inside GetRawIndex sometimes and in the Get/Add methods other times, consistently DCHECK as far up the call stack as possible by deferring the DCHECK to the Get/Add methods, through a new TryGetRawIndex that passes through untagged id indices such as None.

It looks like, before this change, GetRawIndex would always CHECK-fail on the index because every implementation deferred to ValueStore::GetRawIndex, and that implementation contained a CHECK-fail. In other words, maybe there was no reason to have callers add a CHECK-fail because it was centrally implemented.

Am I misunderstanding something? @dwblaikie do you have thoughts on this change?

yes but it took me extra work to understand what had gone wrong every time I ended up in a failure in GetRawIndex() instead of in Add/Get, so I would really prefer to keep them up at the higher level in the stack.

[jonmeow]

On #6308, you say:

Because I hit checks inside GetRawIndex which were less obvious what was going on than in Add().

I assume you're referring to:

CARBON_DCHECK(index >= 0, "{0}", index);

It looks like the main difference is you're printing id, whereas that CHECK was printing index -- but would it help if that CHECK were modified to show the id instead (it appears to have the templated IdT as a the parameter)?

[danakj]

On #6308, you say:

Because I hit checks inside GetRawIndex which were less obvious what was going on than in Add().

I assume you're referring to:

CARBON_DCHECK(index >= 0, "{0}", index);

It looks like the main difference is you're printing id, whereas that CHECK was printing index -- but would it help if that CHECK were modified to show the id instead (it appears to have the templated IdT as a the parameter)?

I think it would help definitely. But it still feels misleading, I keep reading it as being a problem related to tagging somehow, as it's in the tagging helper functions, rather than being at the entry point where I passed in the bad ID.

[dwblaikie]

I'd tend to prefer to keep the check as low as it can go, to reduce the chance it gets missed in some callers in the future.

Would rephrasing/adding some more words to the check help with the contextual confusion?