Skip to content

Conversation

@gasche
Copy link
Member

@gasche gasche commented Jul 16, 2024

I made a mistake when introducing caml_process_pending_actions_res (#13013), currently in trunk caml_process_pending_actions_exn is exported in signals.h but its implementation is removed.

This function is not CAML_INTERNALS, and Coq uses it.

To restore this function I reintroduced a helper function to transform caml_result values back into encoded exception (earlier #13013 designs had this). It is named caml_result_get_encoded_exception, in fail.h, CAML_INTERNALS-only.

Copy link
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.

I don't see the need for a helper function caml_result_get_encoded_exception. I'd rather see it inlined in the function. But it's up to you.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM

I made a mistake when introducing caml_process_pending_actions_res (ocaml#13013), currently in trunk `caml_process_pending_actions_exn` is exported in signals.h but its implementation is removed.

This function is not CAML_INTERNALS, and Coq uses it.

To restore this function I reintroduced a helper function to transform `caml_result` values back into encoded exception (earlier ocaml#13013 designs had this). It is named `caml_result_get_encoded_exception`, in fail.h, CAML_INTERNALS-only.
@gasche gasche force-pushed the restore-caml_process_pending_actions_exn branch from 3b36e72 to 65eaf62 Compare July 17, 2024 19:40
@gasche
Copy link
Member Author

gasche commented Jul 17, 2024

This needs a maintainer approach, maybe @Octachron?

#ifdef CAML_INTERNALS
// internals only, provided for backward-compatibility
Caml_inline value caml_result_get_encoded_exception(
struct caml_result_private result)
Copy link
Member

Choose a reason for hiding this comment

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

Is this function supposed to be part of the puvlic API of caml_result?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it is CAML_INTERNALS only. It is only useful in transition scenarios, but I suspect that we may want to use it in more parts of the runtime occasionally (which is why factored it out of caml_process_pending_actions_exn).

Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

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

I am green-ticking on behalf of @gadmm and @dustanddreams .

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants