Re: C++ question on wrapper API for setting Guile fluids

[Luca Fascione]

On Thu, Apr 21, 2022 at 11:46 PM Jean Abou Samra <jean@abou-samra.fr> wrote:

> Well, the C++ and Scheme interfaces can feel different and idiomatic
> in their respective languages as long as they share the same
> underlying implementation.
>

I think this is a super important goal. In fact, I'd upgrade 'can' to
'should' :-)


> In the same vein, although Guile has scm_dynamic_wind (), the
> recommended C interface is scm_dynwind_begin () / scm_dynwind_end ()
> because that is more scalable in C code.
>

Yes, and that supports your idea of using RAII to bookend it correctly in
the face of exceptions and such things.
Which is very idiomatic in modern C++ and feels quite natural to me.

Dan had an objection about this case:

    {
      Dynwind_context dwc;
      // . . .

      {
        Dynwind_context dwc2;
        // . . .
        dwc.free (p);  // not what it seems
        // . . .
      }
    }

Where the problem is that dwc.free(p) is actually effectively acting as if
it was dwc2.free(p); because the API doesn't pass around the context like
the C++ wrappers appear to do, rather it statefully "just goes for it".
This is a design decision of Guile, obviously. However, it seems to me this
has possibly uncommon semantics even if it were implemented on the
scheme-language side of things anyways, doesn't it? I guess I'm asking
whether people would want/need to do this on purpose, and why. It seems to
me to achieve this behaviour one would have to capture the dwc context and
then invoke dwc.free() on that context from deeper inside its own stack,
and I'm not clear what this means for the frames intervening inbetween the
calling frame and the callee frame, being a current parent of it, maybe
it's ok, I normally think of continuations as things to deal with a stack
that has been unwound, not deeper parts of a stack that is still being
executed. Whereas in a way this is what {} scoping does, it's weird that
there could be intervening function calls and this would still be allowed,
because this second scenario is closer to implementing TCL's upvar/uplevel
mechanism, which I've learned Schemers really don't like. For these reasons
I suspect that Dan's example can easily happen by mistake, but wouldn't be
something that folks have a legitimate, every-day use for, or is it?

Anyways I was reading the source of Guile from the repo, and I found that
dynwind_begin() does this:

void
scm_dynwind_begin (scm_t_dynwind_flags flags)
{
  scm_thread *thread = SCM_I_CURRENT_THREAD;
  scm_dynstack_push_frame (&thread->dynstack, flags);
}


So I'm thinking that (maybe in some appropriate debug mode) if
Dynwind_context were to capture a copy of &thread->dynstack, it could then
check if the stack has moved or not,
and error out in some useful way if the (dyn)stack pointer has changed.
Given C++ semantics, I suspect it can only change by being deeper, but one
way or the other, this could work.
Mind you, as much as the proposed RAII idea is kinda slightly dangerous, I
don't think we should protect too much for folks doing irresponsible things
with the stack, as long as
the documentation is clear about how the wrapper works, this should be
fine.

On the other hand, if this concept of messing with dwc from a stack frame
deeper inside is important, dynstack.h has all controls for capturing and
standing up local stacks, so one could instead do a full capture in the
constructor (say calling scm_dynstack_capture_all, or something like that)
to allow for this behaviour (and then the forwarding methods would adjust
the dynstack to always be pointed as expected).
It seems a bit overkill to me, because I'm not used to the semantics
implied by doing this, but all the same it appears this might work.

-> footnote: these arguably all appear to be internal API entrypoints, I'm
not sure what this means in terms of it being a polite thing or not to call
them from lilypond's codebase.

Earlier in the thread Jean said:

> Making a method of the context is an attractive proposal, as
> it prevents from forgetting to introduce a context.
>
> Actually, the specific use case is much narrower than this particular
> set of methods: I don't need C++ wrappers for all dynwind-related Guile
> APIs, I just have one need for fluids. The example in the first email was
> a straw man for the sake of explanation.

I think the first observation is very good: making it easiest to spot wrong
code without paying large amounts of attention has been a very good
strategy for me in the past.
And for the same reason, assuming the scm_dynwind_XXX set of calls is
small, I would either wrap all of them (because they're all small and easy)
or very clearly document the wrapper class as "if you need this method, add
it here and this is how": you definitely don't want to find yourself in a
halfway house where there are all sorts of exceptions that "yes these three
methods could have been wrapped but we didn't" and now nobody remembers any
longer why a decision was taken one way or the other. It's a small cost now
and can save many headaches later.
Obv this works if the set of scm_dynwind_XXX is small (maybe up to 10-15
calls?), if you have several dozens, you'd definitely wrap a few important
ones that you need, and leave the rest for another day (with the comment
inviting the next person along to help with the effort).

As to the exchange Jean and I were having about non-local control flow, I
was worried about scm_dynwind_end() being called one extra time (rather
than not being called enough, as your example demonstrates) and how to
detect that, but looking at how scm_dynamic_wind() is implemented, maybe
from the C side this can't happen. I'm asking: is there a case where
evaluating the "main", non-guard thunk implies a call to scm_dynwind_end(),
so that the outer C code doesn't need to? And I think the answer to that is
no. So I guess as long as we can guarantee that the think doesn't use
call/cc (or that we always use SCM_F_DYNSTACK_FRAME_REWINDABLE) this will
be ok?
Cheers
L