fix: Termination override check for certain non-reference trait implementations #5087
Conversation
There was a problem hiding this comment.
Thanks for fixing this.
I have two suggestions for additional tests. One is to add the test from #4969 (and to mark this PR as fixing that Issue, too).
The other is to add a test that involves type parameters. This would show that the updated substitution maps / types passed to FunctionCallSubstituter are working as expected (and will cause the test suite to break if that code is changed in the future).
| var T = (TraitDecl)(overryd is Function f1 ? f1.EnclosingClass : ((Method)overryd).EnclosingClass); | ||
| var I = (TopLevelDeclWithMembers)(original is Function f2 ? f2.EnclosingClass : ((Method)original).EnclosingClass); |
There was a problem hiding this comment.
Instead of the ? :, cast to MemberDecl and then take its .EnclosingClass. Like this:
((MemberDecl)overryd).EnclosingClassThere was a problem hiding this comment.
Thanks I tried for some time, but not hard enough, as it turns out.
| for (int i = 0; i < N; i++) { | ||
| Expression e0 = calleeDecreases[i]; | ||
| Expression e1 = Substitute(contextDecreases[i], null, substMap, typeMap); | ||
| sub ??= new FunctionCallSubstituter(substMap, typeMap, T, I); |
There was a problem hiding this comment.
You can leave this as is, but I don't think the extra lines here are worth the saving of creating sub only once and only if needed. If you agree, you could either always initialize sub before the loop (which is more costly only when N is 0) or create a new sub in each iteration (which is more costly only if N is 2 or more). I'd probably go for the latter.
There was a problem hiding this comment.
I tried inlining the construction of the substituter, but even I think it a little much for a single line. Without it, we're only saving a single line.
Closes #4969 and #4982.