Refactor InstNamer::GetScopeIdOffset() to make it easier to comprehend and maintain
#6159
Conversation
…end and maintain Instead of having a switch case with `fallthrough` where each case adds the number of entities of the next id type case, have an array that maps id type to a function that gets the number of the matching entities. This array is less confusing and easier to maintain to avoid bugs like the one fixed in carbon-language#6151. The logic that sums only the entities above the given id is now a common logic so no need to change it when changing the set of ids.
|
To clarify my comment, before, the code block was: Now it looks like the code block is: Both of these seem similar to me, with a couple differences; I always find lambdas add complexity for me, but also the new code has more general framework (i.e. more to understand). So where you say: "The logic that sums only the entities above the given id is now a common logic so no need to change it when changing the set of ids." I don't see how the two are much different: there's a code block for a given group, and additions need an addition. I completely understand that you may find it easier to reason about, but to me the addition of a struct and complex control flow is not fundamentally easier to reason about. If you want something simpler, I'd suggest a solution that drops both the struct and lambdas. |
| offset += sem_ir_->cpp_overload_sets().size(); | ||
| [[fallthrough]]; | ||
| case ScopeIdTypeEnum::For<CppOverloadSetId>: |
There was a problem hiding this comment.
What if we grouped these things with whitespace so that you saw the things above the case are related to it more clearly?
Instead of having a switch case with
fallthroughwhere each case adds the number of entities of the next id type case, have an array that maps id type to a function that gets the number of the matching entities.This array is less confusing and easier to maintain to avoid bugs like the one fixed in #6151.
The logic that sums only the entities above the given id is now a common logic so no need to change it when changing the set of ids.