Avoid deep recursion for nested bound patterns #80920
Conversation
AlekseyTs
changed the title
|
@dotnet/roslyn-compiler Please review |
|
I will try to repro the stack overflow again with this PR and will review it soon. Thanks #Resolved |
|
@dotnet/roslyn-compiler Please review |
333fred
left a comment
333fred
left a comment
There was a problem hiding this comment.
Done review pass. Only a couple of minor questions.
| return false; | ||
| } | ||
|
|
||
| Debug.Assert(t2 is SequenceTests); |
There was a problem hiding this comment.
Nit: it's not obvious at first read that equalsEasyOut will validate that RemainingTests are equal in length. Think we can strengthen this assert and make it easier to understand by asserting that the remaining tests lengths are equal here. #Resolved
|
|
||
| if (!sequence.RemainingTests.Any(t => t is SequenceTests)) | ||
| { | ||
| return sequence.RemainingTests.SequenceEqual(other.RemainingTests); |
There was a problem hiding this comment.
Is there any chance that there will be nested sequences in an individual remaining test? And if so, is that something we want to handle? If the answer to either question is no, consider leaving a comment to explain. #Resolved
There was a problem hiding this comment.
I am not sure what is there to explain. This operation is equivalent to what the "non-easy" comparison would do.
There was a problem hiding this comment.
If the recursion was sequence->nonsequence->sequence..., then we'd still have deep recursion here. That's what I'm concerned about.
There was a problem hiding this comment.
This PR doesn't attempt to avoid that form of recursion, even if one is possible.
|
|
||
| static int? getHashCodeEasyOut(SequenceTests sequence) | ||
| { | ||
| if (sequence.RemainingTests.Any(t => t is SequenceTests)) |
There was a problem hiding this comment.
Same question about nested sequences. #Resolved
There was a problem hiding this comment.
Same answer. I am not making any assumptions about nested sequences.
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM modulo the pending comments. It looks like the stack overflow from #80840 does not repro with this change.
3 participants
Closes #80840.