PATCHES - Countdown to April 24th
Here is the current countdown list. The next countdown will be on April 24th. A list of all merge requests can be found here: https://gitlab.com/lilypond/lilypond/-/merge_requests?sort=label_priority Push: !1317 Fix and improve formatting of Scheme signatures in IR - Jean Abou Samra https://gitlab.com/lilypond/lilypond/-/merge_requests/1317 !1315 Improve (and run) fixscm.sh - Jean Abou Samra https://gitlab.com/lilypond/lilypond/-/merge_requests/1315 !1313 relocate: Try looking through symlinks - Jonas Hahnfeld https://gitlab.com/lilypond/lilypond/-/merge_requests/1313 !1312 Improve build messages - Werner Lemberg https://gitlab.com/lilypond/lilypond/-/merge_requests/1312 Countdown: !1320 Doc: use HTTPS throughout - Jean Abou Samra https://gitlab.com/lilypond/lilypond/-/merge_requests/1320 !1319 Doc-CG: revise 'Further Git documentation resources' - Jean Abou Samra https://gitlab.com/lilypond/lilypond/-/merge_requests/1319 !1318 Grob::internal_get_object: remove a check for unpure/pure containers - Jean Abou Samra https://gitlab.com/lilypond/lilypond/-/merge_requests/1318 !1316 Vim: set undo_ftplugin and undo_indent variables - Doug Kearns https://gitlab.com/lilypond/lilypond/-/merge_requests/1316 Review: !1321 Fix DurationLine thickness for different staff sizes - Thomas Morley https://gitlab.com/lilypond/lilypond/-/merge_requests/1321 !1314 breathMarkType - Dan Eble https://gitlab.com/lilypond/lilypond/-/merge_requests/1314 New: No patches in New at this time. Waiting: !1136 \break implies \bar "" when needed - Dan Eble https://gitlab.com/lilypond/lilypond/-/merge_requests/1136 !913 release: binaries with cairo - Han-Wen Nienhuys https://gitlab.com/lilypond/lilypond/-/merge_requests/913 Cheers, Colin
Re: C++ question on wrapper API for setting Guile fluids
On Apr 22, 2022, at 05:39, Jean Abou Samra wrote:
>
> You both made a good point. My conclusion is that I should stick to
>
> {
> Local_assumption la (Lily::prebreak_estimate, SCM_BOOL_T);
> ...
> }
>
> where the constructor does everything, included scm_dynwind_begin ()
> and scm_dynwind_fluid (). That's safer and clearer, and never mind that
> you have to construct 3 contexts if you need 3 assumptions. I expect
> this to be very rare anyway.
Fine. At the time someone wants more than one assumption, we might be able to
add
Local_assumptions la {{fluid1, value1},
^ {fluid2, value2} /* etc. */ };
where the constructor accepts std::initializer_list> but
I don't know if there are any problems related to GC (and I'm not planning to
work through that now).
—
Dan
Re: Missing dependencies in doc build after addition of PDF syntax highlighting?
Le 21/04/2022 à 09:31, Jonas Hahnfeld a écrit : This says "to rebuild only 'contributor.pdf'", which should still work fine in an incremental build, no? Granted. The first of the two occurrences on that page does not imply that the full documentation build should be done first, though. Jean
Re: C++ question on wrapper API for setting Guile fluids
Jean Abou Samra writes:
> Re call/cc: I would actually like to use
> static_cast (0)
> and not SCM_F_WIND_EXPLICITLY. I have no idea whether the stuff I write
> is going to interact gracefully with rewinding the stack via continuations,
> and I honestly don't want to wonder.We're using C++ (on contrary to Guile,
> which is primarily tailored for C), so most allocations (e.g., vectors) are
> managed via C++ RAII. This means that C++'s stack unwinding and
> Guile's don't
> interact, so you can easily crash LilyPond with continuations and
> such, by making
> some expression return twice and arranging for it to cause double free. This
> is the first example that came to my mind:
>
> \version "2.23.7"
>
> {
> \override NoteHead.X-extent =
> #(let ((cont #f))
> (lambda (grob)
> (if cont
> (cont '(0 . 0))
> (call/cc
> (lambda (c)
> (set! cont c)
> '(0 . 0))
> c d
> }
>
>
>
> Frankly, I don't think we need to bother at all about such stuff. If
> the user is willing this hard to shoot themselves in the foot, we
> can't prevent them.
It's worth keeping in mind that dynamic programming (like trying to
figure out the best breakpoint sequence) naturally lends itself to
juggling a set of best continuations so far (a continuation is more
versatile to build upon than a static description of a partial
solution).
But the main control flow strategies are certainly maintained by
LilyPond itself, so we are mainly shooting ourselves in the foot by
blocking certain programming practices that make your head ache but may
actually be a good fit for "under the hood". So it's stuff that there
is no point in bending over backwards to get right, but if we see a
reasonably easy and affordable way to do it "continuation-safe", it's
probably worth doing it and thus keeping more options easily accessible
for the future.
One example of "weird control flow" I had to revert to is
map-markup-commands even if it does not go to the depths of restarting
continuations but "only" throws parts of a solution.
--
David Kastrup
Re: C++ question on wrapper API for setting Guile fluids
Le 22/04/2022 à 09:52, Luca Fascione a écrit :
[snipped]
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.
I guess it would be possible to make it work, using
scm_current_dynamic_state
and scm_set_current_dynamic_state. I don't have a need to implement it,
though.
Assumptions should always be introduced just after creating the dynamic
context.
[...]
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).
You both made a good point. My conclusion is that I should stick to
{
Local_assumption la (Lily::prebreak_estimate, SCM_BOOL_T);
...
}
where the constructor does everything, included scm_dynwind_begin ()
and scm_dynwind_fluid (). That's safer and clearer, and never mind that
you have to construct 3 contexts if you need 3 assumptions. I expect
this to be very rare anyway.
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?
Re "can scm_dynwind_end () be called unpaired in the thunk": if the callee
respects programming contracts, no. The principle of scm_dynwind_begin
() and
scm_dynwind_end () is that they should always be paired under normal
circumstances
(when no Guile non-local control flow happens).
Re call/cc: I would actually like to use
static_cast (0)
and not SCM_F_WIND_EXPLICITLY. I have no idea whether the stuff I write
is going to interact gracefully with rewinding the stack via continuations,
and I honestly don't want to wonder.We're using C++ (on contrary to Guile,
which is primarily tailored for C), so most allocations (e.g., vectors) are
managed via C++ RAII. This means that C++'s stack unwinding and Guile's
don't
interact, so you can easily crash LilyPond with continuations and such,
by making
some expression return twice and arranging for it to cause double free. This
is the first example that came to my mind:
\version "2.23.7"
{
\override NoteHead.X-extent =
#(let ((cont #f))
(lambda (grob)
(if cont
(cont '(0 . 0))
(call/cc
(lambda (c)
(set! cont c)
'(0 . 0))
c d
}
Frankly, I don't think we need to bother at all about such stuff. If the
user
is willing this hard to shoot themselves in the foot, we can't prevent them.
Echoing Han-Wen's fear about the problems linked with dynamic states,
the reason
to use fluids, apart from consistency with the existing (*parser*) and
(*location*),
is to be a little cooperative with basic and actually useful non-local flow
such as (mock
Re: C++ question on wrapper API for setting Guile fluids
On Thu, Apr 21, 2022 at 11:46 PM Jean Abou Samra 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 (>dynstack, flags);
}
So I'm thinking that (maybe in some appropriate debug mode) if
Dynwind_context were to capture a copy of >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
