MPR#7702: refresh raise counts when inlining a function#1553
MPR#7702: refresh raise counts when inlining a function#1553gasche merged 6 commits intoocaml:trunkfrom
Conversation
|
@lthls: would you by chance be willing to review it (if you believe it is correct, feel free to "approve" the PR.)? |
lthls
left a comment
lthls
left a comment
There was a problem hiding this comment.
This patch looks correct to me, so I approve it on this basis.
However, it would be nice to have added the example from MPR#7702 to the testsuite, and the comment starting at line 502 should probably be updated.
asmcomp/closure.ml
Outdated
| Ustaticfail (nfail, List.map (substitute loc fpc sb) args) | ||
| let nfail = | ||
| match rn with | ||
| | Some rn -> (try Tbl.find nfail rn with Not_found -> nfail) |
There was a problem hiding this comment.
I'm a bit surprised by the try-with statement. In my version it was needed for the let-rec case, but since you only use a renaming for inlining direct applications I believe it should be an error if nfail isn't bound in rn.
There was a problem hiding this comment.
Indeed - replaced the exception handler with a fatal error.
|
No need to rebase; can I let you merge? |
|
The CI is failing because you need a Changes entry. |
|
Here it is. |
Changes
Outdated
| (Jacques Garrigue, report by Nicolas Ojeda Bar) | ||
|
|
||
| - MPR#7702, GPR#1553: refresh raise counts when inlining a function | ||
| (Vincent Laviron, Xavier Clerc, report by Cheng Sun, review by Gabriel Scherer) |
There was a problem hiding this comment.
please remove my name, I haven't reviewed this PR -- I merely passed @lthls' approval.
There was a problem hiding this comment.
OK; I thought the reviewer was the one adding the green badge to a PR.
There was a problem hiding this comment.
The green badge should only happen after serious review work has been performed, and in general the two people coincide (reviewing and badging), but not in this particular case. In general "review by" is useful to credit everyone who contributed useful comments during the review period.
There was a problem hiding this comment.
Understood; could you squash/merge?
|
What about back-porting to 4.06 ? |
MPR#7702: refresh raise counts when inlining a function (ocaml#1553)
* Adds guard type to Parsetree * Pattern guards parsing implemented * format: remove parens Co-authored-by: Nick Roberts <[email protected]> * formatting fixes * test documentation * removes ast dump from tests * match precedence CR * removed catch-all case from guard match --------- Co-authored-by: Nick Roberts <[email protected]>
5 participants
Following the suggestion from @lthls in Mantis#7702, here is a tentative fix.
As the title suggests, it simply refreshes raise counts to ensure there is no clash
when a function is inlined. A new parameter, namely
rn, is added to thesubstitutefunction; its type is
(int, int) Tbl.t option(Noneindicating that counts should notbe refreshed).