Re: [PATCH v1 2/5] tools/ocaml/libs/xc: add binding to xc_evtchn_status

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 1 Dec 2022, at 13:59, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
> 
> On 01/12/2022 13:35, Edwin Torok wrote:
>>> On 1 Dec 2022, at 11:34, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> 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;
> }
> 


I wouldn't expect it to, or at least I've never seen a C binding written that 
way (with CAMLreturn not as last statement),
but indeed nothing in the manual states that it wouldn't work.

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

I almost always used to use do/while(0) and break even in C as a replacement 
for 'goto',
especially if there are multiple nested resources that need cleaning up, 
do/while ensures you
unwind them in the correct order and don't accidentally skip one.
I think most code that is written using 'goto' can be rewritten not to use it, 
and might avoid some bugs in the process
(e.g. using goto might leave some local variables uninitialised).
I'm reluctant to introduce a goto just to decrease nesting level.

There might be another way to rewrite the code:
```
switch(status.status)
{
case EVTCHNSTAT_closed:
 stat = Val_none;
 break;
... other code that assigns to stat something other than None ...
}

if (Val_none == stat) {
   result = Val_none;
} else {
   .. code as it was before to construct a some ...
}

CAMLreturn(result);
```

This would then follow the logical order of how the values are constructed, and 
avoids the deep nesting of the switch.
(reading the code backwards from exit will show you how each piece is 
nested/constructed without jumps that alter control flow)

Val_none == is used instead of == Val_none to catch a typo where stat = 
Val_none would change stat, whereas Val_none = stat would be a compile error.

What do you think?

(might be slightly less efficient than the original version, but a reasonable C 
compiler should produce almost equal optimized code for both).

> 
>>> 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