On 01/12/2022 13:35, Edwin Torok wrote:
>> On 1 Dec 2022, at 11:34, Andrew Cooper <andrew.coop...@citrix.com> wrote:
>>
>> On 30/11/2022 17:32, Edwin Török wrote:
>>> +
>>> + caml_enter_blocking_section();
>>> + rc = xc_evtchn_status(_H(xch), &status);
>>> + caml_leave_blocking_section();
>>> +
>>> + if ( rc < 0 )
>>> + failwith_xc(_H(xch));
>>> +
>>> + if ( status.status == EVTCHNSTAT_closed )
>>> + result = Val_none;
>>> + else
>>> + {
>> This is actually one example where using a second CAMLreturn would
>> simply things substantially.
>>
>> switch ( status.status )
>> {
>> case EVTCHNSTAT_closed:
>> CAMLreturn(Val_none);
>>
>> case EVTCHNSTAT_unbound:
>> ...
>>
>> Would remove the need for the outer if/else.
>
> CAMLreturn has some macro magic to ensure it gets paired with the toplevel
> CAMLparam correctly (one of them opens a { scope and the other closes it, or
> something like that),
> so I'd avoid putting it into the middle of other syntactic elements, it might
> just cause the build to fail (either now or in the future).
From the manual:
"The macros CAMLreturn, CAMLreturn0, and CAMLreturnT are used to replace
the C keyword return. Every occurrence of return x must be replaced by ..."
It is common in C to have multiple returns, and the manual does say
"Every occurrence" without stating any requirement for there to be a
single occurrence.
CAMLreturn can't syntactically work splitting a scope with CAMLparam,
because then this wouldn't compile:
CAMLprim value stub_xc_evtchn_status(value foo)
{
CAMLparam1(foo);
int bar = 0;
retry:
if ( bar )
CAMLreturn(foo);
bar++
goto retry;
}
CAMLreturn does use a normal "do { ... } while (0)" construct simply for
being a macro, and forces paring with CAMLparamX by referencing the
caml__frame local variable by name.
So I really do think that multiple CAMLreturns are fine and cannot
reasonably be broken in the future.
But if we do really still want to keep a single return, then a "goto
done" would be acceptable here and still better than the if/else.
>> caml_alloc_some() is perhaps less interesting as it only appeared in
>> Ocaml 4.12 AFAICT, but we could also have some ifdefary for that at the
>> top of the file.
>>
>> I don't know whether we have opencoded options elsewhere in the
>> bindings, but it certainly would be reduce the amount opencoding that
>> exists for standard patterns.
>
> perhaps we can look into doing that cleanup as a separate patch.
Probably best, yes.
~Andrew
