Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[hanwenn]

On 2020/02/04 21:15:44, dak wrote:
> NLGTM
> 
> This patch just punts on using an internal function, and that could be
equally
> well be done without involving syntax-case.
> 
> An alternative proposal that just relies on a single external symbol
in order to
> achieve the original design is given as
> 
> Tracker issue: 5735
(https://sourceforge.net/p/testlilyissues/issues/5735/)
> Rietveld issue: 549510043 (https://codereview.appspot.com/549510043)
> Issue description:
>   Rewrite define-session and define-session-public macros  The byte
>   compiler of Guile-2.x is not able to compile anonymous
>   functions/closures into constants.  Rewriting the macros not to rely
>   on such constants by moving the local functions into their own
>   function definitions is the easiest expedient.
> 
> https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm
> File scm/lily.scm (right):
> 
>
https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode135
> scm/lily.scm:135: (acons (quote name) (make-session-variable (quote
name) value)
> lilypond-exports))
> This is actually a purely textual replacement rather than anything
more complex
> or hygienic.  As such it does nothing that cannot equally well be
achieved using
> defmacro: the problematic issue with the byte compiler was the
internal function
> that was used for _avoiding_ a wholesale textual replacement.
> 
> So drawing in a module that is known to be buggy and unmaintained in
Guile-1.8
> does not even serve a purpose here.

closing in favor of https://codereview.appspot.com/549510043/ - you have
my permission to skip countdown :-)

https://codereview.appspot.com/553480044/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[dak]

NLGTM

This patch just punts on using an internal function, and that could be
equally well be done without involving syntax-case.

An alternative proposal that just relies on a single external symbol in
order to achieve the original design is given as

Tracker issue: 5735
(https://sourceforge.net/p/testlilyissues/issues/5735/)
Rietveld issue: 549510043 (https://codereview.appspot.com/549510043)
Issue description:
  Rewrite define-session and define-session-public macros  The byte
  compiler of Guile-2.x is not able to compile anonymous
  functions/closures into constants.  Rewriting the macros not to rely
  on such constants by moving the local functions into their own
  function definitions is the easiest expedient.



https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm
File scm/lily.scm (right):

https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode135
scm/lily.scm:135: (acons (quote name) (make-session-variable (quote
name) value) lilypond-exports))
This is actually a purely textual replacement rather than anything more
complex or hygienic.  As such it does nothing that cannot equally well
be achieved using defmacro: the problematic issue with the byte compiler
was the internal function that was used for _avoiding_ a wholesale
textual replacement.

So drawing in a module that is known to be buggy and unmaintained in
Guile-1.8 does not even serve a purpose here.

https://codereview.appspot.com/553480044/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[dak]

On 2020/02/04 08:56:49, hanwenn wrote:
> On 2020/02/03 12:11:08, dak wrote:
> > > What is wrong with the code I propose in this change?
> > 
> > You mean define-syntax?  I'd like to avoid it for now since people
have no way
> > of getting acquainted with it, and most new developers have never
worked with
> > Scheme before.
> 
> I don't understand this argument. If we have no new developers that
know Scheme,
> they wouldn't know what to do with defmacro either? Am I missing
something?

defmacro works in user documents.  define-syntax doesn't.  That gives
people a way to get acquainted with defmacro that doesn't exist yet for
define-syntax .

> I didn't know about define-syntax before writing this patch, and I
learned by 
> googling around for it.



https://codereview.appspot.com/553480044/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[hanwenn]

On 2020/02/03 12:11:08, dak wrote:
> > What is wrong with the code I propose in this change?
> 
> You mean define-syntax?  I'd like to avoid it for now since people
have no way
> of getting acquainted with it, and most new developers have never
worked with
> Scheme before.

I don't understand this argument. If we have no new developers that know
Scheme,
they wouldn't know what to do with defmacro either? Am I missing
something? 

I didn't know about define-syntax before writing this patch, and I
learned by 
googling around for it.


https://codereview.appspot.com/553480044/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[dak]

On 2020/02/03 12:11:08, dak wrote:
> On 2020/02/03 09:49:32, hanwenn wrote:
> > On 2020/02/02 15:28:44, dak wrote:
> 
> > > That's what I am trying right now, but you cannot quote outer
functions as a
> > > value either (the problem is the value, not the innerness) so you
need to do
> > it
> > > by name and I would like not to have to export it, so I try with
(@@
> > guile-user
> > > define-session-internal) but haven't found the right incantation
yet.  Still
> > > fiddling.  If it cannot be avoided, it will end up as an exported
> > > define-session-internal .
> > 
> > I don't get it. define-session is not a public macro either.
> 
> No, _I_ don't get it.  Darn, you are right.  So the cross-module issue
does not
> exist and just using 'define-session-internal should be fine.

To wit: the following boil-down of the example code you submitted to the
Guile list works fine:

(define decl '())
(define (make-var n v) (list "var" n v))
(define (define-session-internal n v)
  (set! decl
(cons
 (make-var n v)
 decl)))

(defmacro define-session (name value)
  `(define-session-internal ',name ,value))

(define-session foo 1)
(display decl)
(newline)

Now that approach will take care of define-session.  Where we have an
_exported_ macro of similar kind, things get uglier.  At the current
point of time, I see no viable path forward other than just exporting
the internal function as well.

https://codereview.appspot.com/553480044/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[dak]

On 2020/02/03 09:49:32, hanwenn wrote:
> On 2020/02/02 15:28:44, dak wrote:

> > That's what I am trying right now, but you cannot quote outer
functions as a
> > value either (the problem is the value, not the innerness) so you
need to do
> it
> > by name and I would like not to have to export it, so I try with (@@
> guile-user
> > define-session-internal) but haven't found the right incantation
yet.  Still
> > fiddling.  If it cannot be avoided, it will end up as an exported
> > define-session-internal .
> 
> I don't get it. define-session is not a public macro either.

No, I don't get it.  Darn, you are right.  So the cross-module issue
does not exist and just using 'define-session-internal should be fine.

> What is wrong with the code I propose in this change?

You mean define-syntax?  I'd like to avoid it for now since people have
no way of getting acquainted with it, and most new developers have never
worked with Scheme before.

> 
> > > What is the problem precisely with void ? I assume that is not
about
> > > define-session but for define-music-function, which looks like it
has a very
> > > complex macro expansion.
> > > 
> > > Can you provide a repro scenario?
> 
> > But basically the symptom was just that using define-syntax in a
typical
> manner
> > from inside of a LilyPond document crashed with some error
completely
> unrelated
> > to the actual input.
> 
> would be good to have an actual patch along with the failing .ly to
examine more
> closely.

I can try again but the only way in which it would be relevant is if we
forked Guile-1.8.8 and started fixing bugs in it.  I've reported it
already but it has been declared as wontfix.  The report is


https://codereview.appspot.com/553480044/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[hanwenn]

On 2020/02/02 15:28:44, dak wrote:
> On 2020/02/02 14:01:33, hanwenn wrote:
> > https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm
> > File scm/lily.scm (right):
> > 
> >
>
https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode111
> > scm/lily.scm:111: "This defines a variable @var{name} with the
starting value
> > On 2020/02/01 21:37:50, dak wrote:
> > > An interesting DOC string method...  define-syntax doesn't have
anything
> like
> > > that?
> > 
> > 
> > no, doesn't look like it.
> > 
> > I added all the GUILE repos I could find to cs.bazel.build, none of
the
> > define-syntax uses seem to have doc strings. See
> > 
> > https://cs.bazel.build/search?q=define-syntax%20f%3ascm%20&num=450
> > 
> >
>
https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode123
> > scm/lily.scm:123: (define-syntax define-session
> > On 2020/02/01 21:37:50, dak wrote:
> > > The problem with define-syntax is that in Guile-1.8, it doesn't
work
> properly
> > > because of some symbol changing behavior when defined, a bug that
will not
> get
> > > fixed any more.  I wish I'd remember what it was.  It was
something short
> like
> > > \once or \temporary or so.  One could use it in the LilyPond core,
but it
> > failed
> > > in user documents.  Ah right: pretty sure it was \void (namely the
symbol
> > 'void
> > > being bound to something).
> > > 
> > > That makes it hard for people to get acquainted with it and
maintain it in
> > > consequence.  So let's see whether we manage to muddle through in
a
> different
> > > manner.  Do you have a useful test case?  Should I just draw out
the
> > > define-session thing and try to convince Guile-2.0 with the right
> incantations
> > > to byte compile it?  I assume this is the same in Guile-2.2 (for
which I
> have
> > an
> > > Ubuntu package I don't need to compile)?
> > > 
> > > Would this give us a chance to get somewhere?
> > 
> > Please see Taylan's reply to the guile-devel list, which gives a
succinct
> > explanation of why inner functions in macros can never work in the
Guile 2.x
> > compiler. So we either forego compilation (at a 1.5 sec startup
cost) or we
> > rewrite the macros to stop using inner functions.
> 
> That's what I am trying right now, but you cannot quote outer
functions as a
> value either (the problem is the value, not the innerness) so you need
to do it
> by name and I would like not to have to export it, so I try with (@@
guile-user
> define-session-internal) but haven't found the right incantation yet. 
Still
> fiddling.  If it cannot be avoided, it will end up as an exported
> define-session-internal .

I don't get it. define-session is not a public macro either. 
What is wrong with the code I propose in this change?

> > What is the problem precisely with void ? I assume that is not about
> > define-session but for define-music-function, which looks like it
has a very
> > complex macro expansion.
> > 
> > Can you provide a repro scenario?

> But basically the symptom was just that using define-syntax in a
typical manner
> from inside of a LilyPond document crashed with some error completely
unrelated
> to the actual input.

would be good to have an actual patch along with the failing .ly to
examine more closely.


https://codereview.appspot.com/553480044/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[Han-Wen Nienhuys]

On Sun, Feb 2, 2020 at 4:28 PM  wrote:

> > Please see Taylan's reply to the guile-devel list, which gives a
> succinct
> > explanation of why inner functions in macros can never work in the
> Guile 2.x
> > compiler. So we either forego compilation (at a 1.5 sec startup cost)
> or we
> > rewrite the macros to stop using inner functions.
>
> That's what I am trying right now, but you cannot quote outer functions
> as a value either (the problem is the value, not the innerness) so you
> need to do it by name and I would like not to have to export it, so I
> try with (@@ guile-user define-session-internal) but haven't found the
> right incantation yet.  Still fiddling.  If it cannot be avoided, it
> will end up as an exported define-session-internal .

I guess we would have to inline the function (effectively changing it
from function to macro.)

-- 
Han-Wen Nienhuys - hanw...@gmail.com - http://www.xs4all.nl/~hanwen

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[dak]

On 2020/02/02 14:01:33, hanwenn wrote:
> https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm
> File scm/lily.scm (right):
> 
>
https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode111
> scm/lily.scm:111: "This defines a variable @var{name} with the
starting value
> On 2020/02/01 21:37:50, dak wrote:
> > An interesting DOC string method...  define-syntax doesn't have
anything like
> > that?
> 
> 
> no, doesn't look like it.
> 
> I added all the GUILE repos I could find to cs.bazel.build, none of
the
> define-syntax uses seem to have doc strings. See
> 
> https://cs.bazel.build/search?q=define-syntax%20f%3ascm%20&num=450
> 
>
https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode123
> scm/lily.scm:123: (define-syntax define-session
> On 2020/02/01 21:37:50, dak wrote:
> > The problem with define-syntax is that in Guile-1.8, it doesn't work
properly
> > because of some symbol changing behavior when defined, a bug that
will not get
> > fixed any more.  I wish I'd remember what it was.  It was something
short like
> > \once or \temporary or so.  One could use it in the LilyPond core,
but it
> failed
> > in user documents.  Ah right: pretty sure it was \void (namely the
symbol
> 'void
> > being bound to something).
> > 
> > That makes it hard for people to get acquainted with it and maintain
it in
> > consequence.  So let's see whether we manage to muddle through in a
different
> > manner.  Do you have a useful test case?  Should I just draw out the
> > define-session thing and try to convince Guile-2.0 with the right
incantations
> > to byte compile it?  I assume this is the same in Guile-2.2 (for
which I have
> an
> > Ubuntu package I don't need to compile)?
> > 
> > Would this give us a chance to get somewhere?
> 
> Please see Taylan's reply to the guile-devel list, which gives a
succinct
> explanation of why inner functions in macros can never work in the
Guile 2.x
> compiler. So we either forego compilation (at a 1.5 sec startup cost)
or we
> rewrite the macros to stop using inner functions.

That's what I am trying right now, but you cannot quote outer functions
as a value either (the problem is the value, not the innerness) so you
need to do it by name and I would like not to have to export it, so I
try with (@@ guile-user define-session-internal) but haven't found the
right incantation yet.  Still fiddling.  If it cannot be avoided, it
will end up as an exported define-session-internal .
 
> What is the problem precisely with void ? I assume that is not about
> define-session but for define-music-function, which looks like it has
a very
> complex macro expansion.
> 
> Can you provide a repro scenario?

\void is used as a prefix when you want to evaluate something but not
have it used, like
\void \displayMusic { c' g' } without getting anything typeset.

There is no obvious issue per se, but the internals of define-syntax in
Guile-1.8 have a bug where it fails when 'void is defined.  When you use
it in a .ly document with
#(define-syntax ...)
it fails with some obscure error.  The problem may also be with
syntax-case: I don't remember the details, but it was quite obscure.  I
don't know whether there are other potential symbols with the same
detrimental effects.

But basically the symptom was just that using define-syntax in a typical
manner from inside of a LilyPond document crashed with some error
completely unrelated to the actual input.

> Another option: we decide that as of LilyPond 2.21, you will need
GUILE 2.2 for
> LilyPond,

Reality check: LilyPond 2.21 will (hopefully) be out within several
weeks.  We might change the basis sometimes _during_ 2.21 (namely in
time before 2.22) but the first 2.21 versions certainly will not depend
on Guile 2.  Not least of all because 2.21.0 will already have far too
many changes for an unstable release.  Cramming in another major change
should really happen at a later 2.21.x .

> and then we can rewrite the Scheme code without having to worry about
> G1.8 bugs.  With the performance tuning on BDWGC, I think this a
viable strategy
> too.

I don't think performing a full-scale forced change that does not allow
us to benchmark the _difference_ is a good strategy.  It would really be
useful to have some period during which either version would work.

As I said: the define-syntax bummer was .ly-level only, and if push
comes to shove, we could even rename \void .  So there is no absolute
prohibition that I know of to use define-syntax in a .scm file outside
of the current parser module.  It's just icky to use constructs not
allowed to the user.



https://codereview.appspot.com/553480044/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[hanwenn]

https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm
File scm/lily.scm (right):

https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode111
scm/lily.scm:111: "This defines a variable @var{name} with the starting
value
On 2020/02/01 21:37:50, dak wrote:
> An interesting DOC string method...  define-syntax doesn't have
anything like
> that?


no, doesn't look like it.

I added all the GUILE repos I could find to cs.bazel.build, none of the
define-syntax uses seem to have doc strings. See

https://cs.bazel.build/search?q=define-syntax%20f%3ascm%20&num=450

https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode123
scm/lily.scm:123: (define-syntax define-session
On 2020/02/01 21:37:50, dak wrote:
> The problem with define-syntax is that in Guile-1.8, it doesn't work
properly
> because of some symbol changing behavior when defined, a bug that will
not get
> fixed any more.  I wish I'd remember what it was.  It was something
short like
> \once or \temporary or so.  One could use it in the LilyPond core, but
it failed
> in user documents.  Ah right: pretty sure it was \void (namely the
symbol 'void
> being bound to something).
> 
> That makes it hard for people to get acquainted with it and maintain
it in
> consequence.  So let's see whether we manage to muddle through in a
different
> manner.  Do you have a useful test case?  Should I just draw out the
> define-session thing and try to convince Guile-2.0 with the right
incantations
> to byte compile it?  I assume this is the same in Guile-2.2 (for which
I have an
> Ubuntu package I don't need to compile)?
> 
> Would this give us a chance to get somewhere?

Please see Taylan's reply to the guile-devel list, which gives a
succinct explanation of why inner functions in macros can never work in
the Guile 2.x compiler. So we either forego compilation (at a 1.5 sec
startup cost) or we rewrite the macros to stop using inner functions.

What is the problem precisely with void ? I assume that is not about
define-session but for define-music-function, which looks like it has a
very complex macro expansion.

Can you provide a repro scenario?

Another option: we decide that as of LilyPond 2.21, you will need GUILE
2.2 for LilyPond, and then we can rewrite the Scheme code without having
to worry about G1.8 bugs.  With the performance tuning on BDWGC, I think
this a viable strategy too.

https://codereview.appspot.com/553480044/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[dak]

https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm
File scm/lily.scm (right):

https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode111
scm/lily.scm:111: "This defines a variable @var{name} with the starting
value
An interesting DOC string method...  define-syntax doesn't have anything
like that?

https://codereview.appspot.com/553480044/diff/557280043/scm/lily.scm#newcode123
scm/lily.scm:123: (define-syntax define-session
The problem with define-syntax is that in Guile-1.8, it doesn't work
properly because of some symbol changing behavior when defined, a bug
that will not get fixed any more.  I wish I'd remember what it was.  It
was something short like \once or \temporary or so.  One could use it in
the LilyPond core, but it failed in user documents.  Ah right: pretty
sure it was \void (namely the symbol 'void being bound to something).

That makes it hard for people to get acquainted with it and maintain it
in consequence.  So let's see whether we manage to muddle through in a
different manner.  Do you have a useful test case?  Should I just draw
out the define-session thing and try to convince Guile-2.0 with the
right incantations to byte compile it?  I assume this is the same in
Guile-2.2 (for which I have an Ubuntu package I don't need to compile)?

Would this give us a chance to get somewhere?

https://codereview.appspot.com/553480044/

Re: Use define-syntax for define-session[-public] (issue 553480044 by hanw...@gmail.com)

[hanwenn]

David?

https://codereview.appspot.com/553480044/