typecore: clarify the backtracking logic of type_label_exp#11900
typecore: clarify the backtracking logic of type_label_exp#11900gasche wants to merge 1 commit intoocaml:trunkfrom
Conversation
|
I'm not convinced that the new code is better than the old. It is nice, indeed, to make sure that the result of |
|
Grmph. I'm a bit disappointed by @goldfirere's assessment because I personally do find the new code more readable than the previous one, but on the other hand no one stood up with a different opinion. In general it's healthier to trust the reviewers more than ourselves on these not-clearly-necessary PRs, so I am going to consider that the case for the proposed change is closed with a "reject". There is still some work left over to move the comments to the old code version. This is TODO for me, so I'm leaving open for now. |
|
To be clear: I'm very happy for others to disagree here. It was a close call for me, and I see the value in both approaches. |
|
At the risk of being Grmph'd at, I think I agree with Richard on this one. At the very least the new code is not obviously easier to read, so probably isn't worth the churn. I'll close now, obviously people can reopen if they disagree or see some way to make the refactoring more obviously clearer. |
|
No, on the contrary, it's helpful to get more feedback. Thanks! |
|
Before we permanently wander off, can I request that the comment about the two tries makes it in? That piece, for me, is still a big improvement over the status quo! |
salvaging the textual bits of ocaml#11900
|
Right, right, I submitted a comment-only PR at #12054. |
salvaging the textual bits of ocaml#11900
This PR is a follow-up to #11536, proposing a code simplification that was first sketched during the review of that PR: #11536 (comment)
The difficulty in the code that is being fixed is an unpleasant interaction between backtracking logic and the new
with_local_levelwrappers:~postaction of the wrapper, so that the results returned to the outer level have been correctly generalized.~postaction is not easy as it is not supposed to return any results, just unit.~postactionThe solution proposed in this PR is to use a local exception to signal the need to backtrack from within the
~postaction to the outer code.I also tried to simplify the code and clarify what is going on, with more comments and some code factorization.
Review
I believe that it is better to be familiar (not necessarily expert) with the type-checking code, or interested in learning it on the fly, to review this PR. There is code moving in and out of the
with_local_levelcallback or~postaction, so it is not easy to convince oneself that the behavior is preserved without knowing these abstractions.cc @garrigue @t6s, or possibly @goldfirere