|
|
DescriptionUse define-syntax for define-session[-public]
defmacro with inner functions are not supported in GUILE 2.x as the
inner functions only exist at expansion time, and not at runtime.
This gets rid of the error
unhandled constant #<procedure ... >
during autocompile
Patch Set 1 #Patch Set 2 : Guile 1.8 #
Total comments: 5
MessagesTotal messages: 13
Guile 1.8
Sign in to reply to this message.
David?
Sign in to reply to this message.
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?
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
On Sun, Feb 2, 2020 at 4:28 PM <dak@gnu.org> 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 - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen
Sign in to reply to this message.
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.
Sign in to reply to this message.
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://debbugs.gnu.org/cgi/bugreport.cgi?bug=17096>
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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 :-)
Sign in to reply to this message.
|