这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@liam923
Copy link
Contributor

@liam923 liam923 commented Jun 26, 2025

This PR is a bit non-standard because I did the import in two phases. ie, I ran imported changes at some revision in between minus11 and minus12, and then imported the changes at minus12. As a result, it's probably best to review in two phases: the conflict resolution from the first merge, and then the conflict resolution from the second merge.

The first two commits, cbeeb72 and 5580042, are the result of running the import script and should be skipped. Commits 4153ae7 and a776534 correspond to the second running of the import script and should also be skipped. So here's my overall review recommendation:

  1. Review commits 6f76550 to 4bfc746 (in one chunk, not one-by-one), which correspond to the first conflict resolution.
  2. Review commits 0a2205f onwards (in one chunk), which correspond to the second conflict resolution.

Both merges were pretty straightforward, but there's a couple things worth commenting on:

  • There's a regression in the handling of parameterized libraries (the regression can be seen by the diff to tests/test-dirs/parameters.t/run.t). @lukemaurer is aware of this but doesn't currently have bandwidth to resolve the issue. But parameterized libraries aren't yet widely used and the regression is minor, so it's okay to merge with this regression.
  • There was one non-obvious merge-conflict resolution in cmt_format.ml regarding record_declaration_dependency. I discussed this with @voodoos on a draft PR in this comment thread: Merge compiler renaming changes #150 (comment)

@liam923 liam923 requested a review from goldfirere June 26, 2025 15:49
@liam923
Copy link
Contributor Author

liam923 commented Jun 26, 2025

There's something funky going on with CI - it seems to be failing to build the compiler. I'm currently investigating, but fyi the tests are passing locally for me.

Hm this problem seems to have fixed itself. I'm not sure why it failed at commit 4bfc746 but succeeded at 00cbc66. Maybe it's because it's now using a tagged compiler version? (I think the answer to why is unimportant and should not block this pr from merging)

@liam923 liam923 merged commit 592abbd into main Jun 26, 2025
1 check passed
@liam923 liam923 deleted the merge-5.2.0minus-12 branch June 26, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants