Skip to content

Document caml_result in the manual (FFI chapter)#13216

Merged
gasche merged 1 commit intoocaml:trunkfrom
gasche:caml_result-manual
Jul 23, 2024
Merged

Document caml_result in the manual (FFI chapter)#13216
gasche merged 1 commit intoocaml:trunkfrom
gasche:caml_result-manual

Conversation

@gasche
Copy link
Copy Markdown
Member

@gasche gasche commented Jun 4, 2024

This PR is a companion to #13013, providing documentation for the new caml_result type and convention in the OCaml manual, FFI chapter.

I mostly removed the documentation of the previous encoded-exceptions API, except for in-passing mentions that it may still be used for compatibility with pre-5.3 codebases. People who need to understand the encoded-exception API will have to look for older versions of the manual.

Comment thread manual/src/cmds/intf-c.etex Outdated
Comment thread manual/src/cmds/intf-c.etex Outdated
Comment thread manual/src/cmds/intf-c.etex Outdated
Comment thread manual/src/cmds/intf-c.etex Outdated
Comment thread manual/src/cmds/intf-c.etex Outdated
Comment thread manual/src/cmds/intf-c.etex Outdated
Comment thread manual/src/cmds/intf-c.etex Outdated
@gasche gasche force-pushed the caml_result-manual branch from bf20db2 to f341901 Compare June 5, 2024 07:41
@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jun 5, 2024

Thanks @dustanddreams and @dbuenzli, I took your comments into account. I also actually checked that the example compiles (not bad, I only wrote caml_alloc2 instead of caml_alloc_2) and used an itemized list for a more structure presentation of the caml_result API.

Comment thread manual/src/cmds/intf-c.etex Outdated
@gasche gasche force-pushed the caml_result-manual branch from f341901 to a54325e Compare June 5, 2024 08:11
Comment thread manual/src/cmds/intf-c.etex Outdated
Copy link
Copy Markdown
Contributor

@NickBarnes NickBarnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me (apart from that one typo).

@gasche gasche force-pushed the caml_result-manual branch from a54325e to 8fdffa8 Compare June 5, 2024 08:49
Copy link
Copy Markdown
Contributor

@gadmm gadmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

You could also add a link to the section ss:c-result in the last paragraph of the section ss:c-exceptions which mentions the caml_exception_* functions.

Comment thread manual/src/cmds/intf-c.etex Outdated
Comment thread manual/src/cmds/intf-c.etex Outdated
Comment thread manual/src/cmds/intf-c.etex Outdated
@ghost
Copy link
Copy Markdown

ghost commented Jun 24, 2024

Shouldn't this get merged soon since #13013 has been?

@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jun 24, 2024

I would certainly be in favor and/but this requires the approval of a maintainer.

(I also need to do another round on the feedback of @gadmm -- thanks! -- but this seems orthogonal to approval.)

Comment thread manual/src/cmds/intf-c.etex Outdated
@gasche gasche force-pushed the caml_result-manual branch from 8fdffa8 to 058092d Compare July 19, 2024 21:29
@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jul 19, 2024

I took the comments of @gadmm and @MisterDA into account (thanks!) and rebased the PR. This still needs a maintainer approval to be merged.

@shindere
Copy link
Copy Markdown
Contributor

shindere commented Jul 23, 2024 via email

@gasche gasche force-pushed the caml_result-manual branch from 058092d to b4a84df Compare July 23, 2024 14:39
@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jul 23, 2024

Thanks @shindere! When I restarted working on this PR I had forgotten that it had its own Changes entry, so I used commit metadata (which I don't normally use) to credit reviewers. This is fixed now -- the Changes has all names, and I removed the Reviewed-by tag, nothing against them but they are not standard practice in OCaml land so there is little point in using them just once.

@gasche gasche merged commit b83ac29 into ocaml:trunk Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants