Properly set the name for C++ overload set instructions in SemIR #6156
Conversation
…etScopeIdOffset()` After this change, we correctly increment the offset by the next switch case type. Before this change, we accidentally incremented the offset by `functions()` size instead of `cpp_overload_sets()` size and vice versa. Also sorted the switch cases according to the order of the enum, for consistency which might help prevent a future similar incident. This fix prevents crashing in the newly introduced test `multiple_too_few_args_calls`. This also has the side effect of showing `null name` for `cpp_overload_set_type` and `cpp_overload_set_value`, instead of having an arbitrary name. Examples that deomnstrate the old name is arbitrary can easily be seen in tests like `cpp_namespace.carbon` and `decayed_param.carbon`, but careful review would show that all old names are arbitrary, though often luckily almost make sense.
bricknerb
requested review from
geoffromer and
ivanaivanovska
and removed request for
a team
| // TIP: To dump output, run: | ||
| // TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interop/cpp/function/multiple_too_few_args_calls.carbon | ||
|
|
||
| // --- fail_call_too_few_args.carbon |
There was a problem hiding this comment.
Consider moving this test to an existing file e.g. overloads.carbon, there are already several tests for multiple calls, adding this one could fit well there.
There was a problem hiding this comment.
The tests for multiple calls in overloads.carbon are import_both_overloaded_functions_called import_multiple_overloaded_sets, both of them focus on functions with overloads.
This test functionality doesn't really focus on overloading.
It focuses on calling a function with wrong number of args, which is tested in many test files: cpp_diagnostics, decayed_param, default_arg, method, operators, overloads and reference.
Calling more than one function in a test is covered by many tests, so I don't think this makes overloads a great fit, and I think a reader would find it strange to have a test with no overloads there.
I think this use case is somewhat special since it catches a crash that happens on a combination of circumstances, so I definitely want to have it to make sure the crash doesn't recur and putting it separate as it's not a natural fit elsewhere might help it outlive a cleanup.
…etScopeIdOffset()` (#6151) After this change, we correctly increment the offset by the next switch case type. Before this change, we accidentally incremented the offset by `functions()` size instead of `cpp_overload_sets()` size and vice versa. Also sorted the switch cases according to the order of the enum, for consistency. This might help prevent a future similar incident. This fix prevents crashing in the newly introduced test `multiple_too_few_args_calls`. This also has the side effect of showing `null name` for `cpp_overload_set_type` and `cpp_overload_set_value`, instead of having an arbitrary name. Examples that demonstrate the old name is arbitrary can easily be seen in tests like `cpp_namespace.carbon` and `decayed_param.carbon`, but careful review would show that all old names are arbitrary, though often luckily almost make sense. We might want to have a proper name for these, but it's beyond the scope of this crash fixing change. See #6156. Part of #5915.
This is a followup of #5891.
Part of #5915.