Clean up handling of Call params
#5061
Conversation
| // parameters" terminology in EntityWithParamsBase. | ||
| struct RuntimeParamIndex : public IndexBase<RuntimeParamIndex> { | ||
| // `Call` parameter index. | ||
| struct CallParamIndex : public IndexBase<CallParamIndex> { | ||
| static constexpr llvm::StringLiteral Label = "runtime_param"; |
There was a problem hiding this comment.
Do you want to change this here? Or maybe useful to do in a follow-up patch to isolate the noisy churn?
(If in a follow-up, leave a TODO for this PR?)
There was a problem hiding this comment.
Good catch, yes, we should change this. This PR was already supposed to isolate the noisy churn of dropping all those value_param_patterns (although I admit there's been some scope creep), so it seems simpler to take care of this now.
There was a problem hiding this comment.
So, having seen this, I think it would be better to do this in a follow-up PR. Changing this changes essentially every test file, where-as without this there was a fairly reasonable subset of tests really impacted by the change.
I'm approving, but I'd suggest undoing this and leaving a TODO and having a PR that does just this rename so that it's a bit more isolated. Otherwise, for example, I don't think I could find the interesting diagnostic change in the output.
...a/builtin_conversions/no_prelude/fail_convert_facet_value_shouldnt_know_concrete_type.carbon
Show resolved
Hide resolved
| // parameters" terminology in EntityWithParamsBase. | ||
| struct RuntimeParamIndex : public IndexBase<RuntimeParamIndex> { | ||
| // `Call` parameter index. | ||
| struct CallParamIndex : public IndexBase<CallParamIndex> { | ||
| static constexpr llvm::StringLiteral Label = "runtime_param"; |
There was a problem hiding this comment.
So, having seen this, I think it would be better to do this in a follow-up PR. Changing this changes essentially every test file, where-as without this there was a fairly reasonable subset of tests really impacted by the change.
I'm approving, but I'd suggest undoing this and leaving a TODO and having a PR that does just this rename so that it's a bit more isolated. Otherwise, for example, I don't think I could find the interesting diagnostic change in the output.
*Paramand*ParamPatterninsts representCallparameters.ValueParamPatterninsts (because they aren'tCallparameters).MatchContext::results_relates to theCallparameters, and be more consistent about when it's written to.RuntimeParamIndex::Unknown: we no longer need to distinguish "thisParam's runtime index is unknown" from "thisParamisn't a runtime param", because we no longer useParams at all in the latter case.RuntimeParamIndextoCallParamIndex.As a side effect of removing the
ValueParamPatterninsts, this fixes a minor diagnostic bug whereNoteInitializingParamdidn't identify the specific parameter that led to a deduction failure, because it expects generic parameters to only be represented bySymbolicBindingPatterns, but before this change they could be wrapped inValueParamPatterns.