Skip to content

Export Cmt2annot.{iterator,binary_part}#13228

Merged
nojb merged 1 commit intoocaml:trunkfrom
dra27:cmt2annot
Jun 18, 2024
Merged

Export Cmt2annot.{iterator,binary_part}#13228
nojb merged 1 commit intoocaml:trunkfrom
dra27:cmt2annot

Conversation

@dra27
Copy link
Copy Markdown
Member

@dra27 dra27 commented Jun 10, 2024

Following up #11288 (comment) - there may be some further discussion wanted as to how much support should remain here for annot files, but as a starting point and to track the issue, here's a PR re-exporting those functions again.

cc @garrigue

These two functions are used by ocamlbrowser.
@shindere
Copy link
Copy Markdown
Contributor

This looks good to me, modulo the nitpick that the order of the
val declarations in the .mli could respect the order or the
corresponding let declarations in the .ml.

@garrigue
Copy link
Copy Markdown
Contributor

garrigue commented Jun 10, 2024

Thanks for the fast PR.

I'm still wondering what to do for ocamlbrowser on ocaml 5.2.
This PR will eventually allow to write a 5.3 version of ocamlbrowser, but for now the only solution still seems to be to include a copy of cmt2annot.ml. The license of ocamlbrowser being the same as ocaml, is this OK ? (They were originally distributed together.)
(I just realized that there is no proper license included in the labltk distribution; it is only indicated in the opam package, but I will fix that.)

@gasche
Copy link
Copy Markdown
Member

gasche commented Jun 10, 2024

The license of ocamlbrowser being the same as ocaml, is this OK ?

Yes, this is OK. The people who wrote and then made substantial changes to this file will become contributors of ocamlbrowser, which is fine.

Note: I think that it's not necessarily a bad thing that you adopt this part of the logic and have the opportunity to adopt/refine/simplify it based on the needs of ocamlbrowser. Maybe you will find in the end that this is better than hooking into this specific compiler-libs module, time will tell.

@shindere
Copy link
Copy Markdown
Contributor

shindere commented Jun 10, 2024 via email

@garrigue
Copy link
Copy Markdown
Contributor

Would it help if we'd backport this fix to 5.2, assumingthere will be a 5.2.1 release at some point?

I don't think it is necessary. For this release, compatible with 5.2, I will include cmt2annot.ml in the labltk distribution.
For the next one, based on 5.3, I can then remove it.

@nojb nojb merged commit 1d7d915 into ocaml:trunk Jun 18, 2024
yallop added a commit to yallop/ocaml that referenced this pull request Jun 29, 2024
gasche added a commit that referenced this pull request Jun 29, 2024
Add the missing reviewer entry for PR #13228
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.

5 participants