Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(315)

Issue 1160044: T1055: Avoid using deprecated %module-public-interface in guile initialisation.

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 1 month ago by Ian Hulin
Modified:
8 years, 9 months ago
Reviewers:
Ian Hulin (gmail), Patrick McCarty, carl.d.sorensen, Neil Puttock, hanwenn
CC:
frogs_lilynet.net, lilypond-devel_gnu.org
Visibility:
Public.

Description

T1055: Avoid using deprecated %module-public-interface in guile initialisation. Also rename ly_make_anonymous_module to ly_scope_make_module

Patch Set 1 #

Total comments: 14

Patch Set 2 : Action comments from Carl and Han-Wen #

Patch Set 3 : Correct comment in lily.scm for guile-v2 #

Total comments: 6

Patch Set 4 : Implement feedback from Han-Wen and Patrick #

Total comments: 5

Patch Set 5 : Correct test in lily.scm (guile-2 -> guile-v2) #

Patch Set 6 : Fix problems with cli-regions regression test #

Patch Set 7 : Add changes to allow regression tests to complete #

Total comments: 22

Patch Set 8 : Move declarations from clip-region to output-lib; revert regression test change; fix format handler #

Unified diffs Side-by-side diffs Delta from patch set Stats (+174 lines, -103 lines) Patch
M lily/lily-lexer.cc View 1 2 3 4 5 6 7 2 chunks +4 lines, -1 line 0 comments Download
M lily/ly-module.cc View 1 2 3 4 5 6 4 chunks +39 lines, -19 lines 0 comments Download
M scm/clip-region.scm View 1 chunk +18 lines, -48 lines 0 comments Download
M scm/define-grob-properties.scm View 1 chunk +2 lines, -0 lines 0 comments Download
M scm/lily.scm View 1 2 3 4 5 6 7 10 chunks +61 lines, -35 lines 0 comments Download
M scm/output-lib.scm View 1 chunk +50 lines, -0 lines 0 comments Download

Messages

Total messages: 42
Carl
THanks, Ian. I think this is a real contribution. I appreciate your tackling it. The ...
9 years, 1 month ago (2010-05-11 23:42:22 UTC) #1
hanwenn
looks good overall. Some nits: http://codereview.appspot.com/1160044/diff/1/3 File lily/include/ly-module.hh (right): http://codereview.appspot.com/1160044/diff/1/3#newcode25 lily/include/ly-module.hh:25: SCM ly_scope_make_module (bool safe); ...
9 years, 1 month ago (2010-05-12 02:24:19 UTC) #2
Ian Hulin
I've answered all the comments bar one, Han-Wen could you spell out what you meant ...
9 years, 1 month ago (2010-05-13 20:37:23 UTC) #3
hanwenn
http://www.codereview.appspot.com/1160044/diff/1/6 File lily/ly-module.cc (right): http://www.codereview.appspot.com/1160044/diff/1/6#newcode61 lily/ly-module.cc:61: SCM maker = ly_lily_module_constant ("make-module"); On 2010/05/13 20:37:23, Ian ...
9 years, 1 month ago (2010-05-14 03:23:54 UTC) #4
Ian Hulin
http://www.codereview.appspot.com/1160044/diff/1/6 File lily/ly-module.cc (right): http://www.codereview.appspot.com/1160044/diff/1/6#newcode61 lily/ly-module.cc:61: SCM maker = ly_lily_module_constant ("make-module"); On 2010/05/14 03:23:55, hanwenn ...
9 years, 1 month ago (2010-05-15 19:56:47 UTC) #5
Patrick McCarty
On 2010/05/15 19:56:47, Ian Hulin wrote: > On 2010/05/14 03:23:55, hanwenn wrote: > > > ...
9 years, 1 month ago (2010-05-15 21:39:39 UTC) #6
Patrick McCarty
Hi Ian, I haven't tested your patch yet to make sure everything is okay, but ...
9 years, 1 month ago (2010-05-15 21:53:15 UTC) #7
Patrick McCarty
http://www.codereview.appspot.com/1160044/diff/10001/11008 File scm/lily.scm (right): http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode251 scm/lily.scm:251: (string>? (version) "1.8.7")) On 2010/05/15 21:53:15, Patrick McCarty wrote: ...
9 years, 1 month ago (2010-05-15 21:55:16 UTC) #8
Ian Hulin
On 2010/05/15 21:39:39, Patrick McCarty wrote: > On 2010/05/15 19:56:47, Ian Hulin wrote: > > ...
9 years, 1 month ago (2010-05-16 10:39:14 UTC) #9
Ian Hulin
http://www.codereview.appspot.com/1160044/diff/10001/11008 File scm/lily.scm (right): http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode251 scm/lily.scm:251: (string>? (version) "1.8.7")) On 2010/05/15 21:53:15, Patrick McCarty wrote: ...
9 years, 1 month ago (2010-05-16 10:39:48 UTC) #10
Neil Puttock
http://www.codereview.appspot.com/1160044/diff/20001/16009 File scm/lily.scm (right): http://www.codereview.appspot.com/1160044/diff/20001/16009#newcode250 scm/lily.scm:250: (define-public (guile-v2 ) (define-public (guile-v2) http://www.codereview.appspot.com/1160044/diff/20001/16009#newcode251 scm/lily.scm:251: (string>? (version) ...
9 years, 1 month ago (2010-05-17 22:44:22 UTC) #11
Ian Hulin
Thanks for the catch, Neil, Cheers, Ian http://www.codereview.appspot.com/1160044/diff/20001/16009 File scm/lily.scm (right): http://www.codereview.appspot.com/1160044/diff/20001/16009#newcode251 scm/lily.scm:251: (string>? (version) ...
9 years, 1 month ago (2010-05-18 17:42:27 UTC) #12
Neil Puttock
Hi Ian, I've tried testing your latest patch, but it fails on clip-systems.ly. `make-rhythmic-location' (defined ...
9 years, 1 month ago (2010-05-19 22:42:17 UTC) #13
Ian Hulin
Hi Neil, On 19/05/10 23:42, n.puttock@gmail.com wrote: > Hi Ian, > > I've tried testing ...
9 years, 1 month ago (2010-05-22 19:52:01 UTC) #14
Ian Hulin
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi all, On 19/05/10 23:42, n.puttock@gmail.com wrote: > Hi ...
8 years, 11 months ago (2010-07-28 22:49:11 UTC) #15
Patrick McCarty
Hi Ian, On 2010/07/28 22:49:11, Ian Hulin wrote: > On 19/05/10 23:42, mailto:n.puttock@gmail.com wrote: > ...
8 years, 11 months ago (2010-07-29 00:28:24 UTC) #16
Patrick McCarty
On 2010/07/29 00:28:24, Patrick McCarty wrote: > On 2010/07/28 22:49:11, Ian Hulin wrote: > > ...
8 years, 11 months ago (2010-07-29 00:55:35 UTC) #17
Neil Puttock
On 2010/07/29 00:55:35, Patrick McCarty wrote: > With your patchset, I am getting a compile ...
8 years, 11 months ago (2010-07-29 19:24:46 UTC) #18
Ian Hulin
Hi Patrick, Neil and all, On 29/07/10 20:24, n.puttock@gmail.com wrote: > On 2010/07/29 00:55:35, Patrick ...
8 years, 11 months ago (2010-08-01 00:11:44 UTC) #19
Neil Puttock
On 2010/08/01 00:11:44, Ian Hulin wrote: > How did you guys track it down to ...
8 years, 10 months ago (2010-08-01 11:50:13 UTC) #20
Patrick McCarty
On Sun, Aug 1, 2010 at 1:03 PM, <pnorcks@gmail.com> wrote: > On 2010/07/29 19:24:46, Neil ...
8 years, 10 months ago (2010-08-01 20:49:22 UTC) #21
Neil Puttock
On 1 August 2010 21:49, Patrick McCarty <pnorcks@gmail.com> wrote: > I think I found the ...
8 years, 10 months ago (2010-08-01 21:02:31 UTC) #22
Patrick McCarty
On Sun, Aug 1, 2010 at 2:02 PM, Neil Puttock <n.puttock@gmail.com> wrote: > On 1 ...
8 years, 10 months ago (2010-08-01 21:37:32 UTC) #23
Ian Hulin
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi all, On 01/08/10 22:37, Patrick McCarty wrote: > ...
8 years, 10 months ago (2010-08-04 23:13:59 UTC) #24
hanwenn
On Wed, Aug 4, 2010 at 8:13 PM, Ian Hulin <ian@hulin.org.uk> wrote: > On 01/08/10 ...
8 years, 10 months ago (2010-08-05 00:16:23 UTC) #25
Ian Hulin
Hi all, An Updated patch set is available for review. It's undergone a regression test ...
8 years, 10 months ago (2010-08-05 11:32:08 UTC) #26
Patrick McCarty
http://www.codereview.appspot.com/1160044/diff/51001/52005 File scm/lily.scm (right): http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode215 scm/lily.scm:215: (apply fancy-format (cons dest . rest)))) `cons' only works ...
8 years, 10 months ago (2010-08-09 18:35:53 UTC) #27
Neil Puttock
Hi Ian, Can you remove the tab->space changes you've made (particularly in lily.scm)? Some of ...
8 years, 10 months ago (2010-08-09 19:08:57 UTC) #28
Patrick McCarty
http://www.codereview.appspot.com/1160044/diff/51001/52003 File lily/ly-module.cc (right): http://www.codereview.appspot.com/1160044/diff/51001/52003#newcode46 lily/ly-module.cc:46: Guile V1.9/2.0 On 2010/08/09 19:08:58, Neil Puttock wrote: > ...
8 years, 10 months ago (2010-08-09 19:15:44 UTC) #29
Neil Puttock
On 2010/08/09 19:15:44, Patrick McCarty wrote: > I don't know. But I did file a ...
8 years, 10 months ago (2010-08-09 19:20:50 UTC) #30
Ian Hulin
http://www.codereview.appspot.com/1160044/diff/51001/52001 File input/regression/clip-systems.ly (right): http://www.codereview.appspot.com/1160044/diff/51001/52001#newcode47 input/regression/clip-systems.ly:47: #(use-modules (scm clip-region)) On 2010/08/09 19:08:58, Neil Puttock wrote: ...
8 years, 10 months ago (2010-08-09 20:01:10 UTC) #31
Neil Puttock
On 2010/08/09 20:01:10, Ian Hulin wrote: > Presumably this means make a parallel change to ...
8 years, 10 months ago (2010-08-09 21:34:55 UTC) #32
Ian Hulin
Hi Neil, On 09/08/10 22:34, n.puttock@gmail.com wrote: > On 2010/08/09 20:01:10, Ian Hulin wrote: > ...
8 years, 10 months ago (2010-08-10 19:00:01 UTC) #33
Neil Puttock
On 2010/08/10 19:00:01, Ian Hulin wrote: > Errm, unless you're using an unpatched, vanilla lily.scm ...
8 years, 10 months ago (2010-08-10 21:24:48 UTC) #34
Ian Hulin
http://www.codereview.appspot.com/1160044/diff/51001/52005 File scm/lily.scm (right): http://www.codereview.appspot.com/1160044/diff/51001/52005#newcode215 scm/lily.scm:215: (apply fancy-format (cons dest . rest)))) On 2010/08/09 18:35:54, ...
8 years, 10 months ago (2010-08-11 09:27:41 UTC) #35
Ian Hulin (gmail)
Even though this is title T1055: this change is now being tracked as Issue 1224. ...
8 years, 10 months ago (2010-08-17 22:18:34 UTC) #36
Ian Hulin
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi all, This was the artist formerly known as ...
8 years, 10 months ago (2010-08-28 19:16:18 UTC) #37
Neil Puttock
On 2010/08/28 19:16:18, Ian Hulin wrote: > Anyhow, building against V1.9.11 showed up Neil's concerns ...
8 years, 10 months ago (2010-08-28 19:36:27 UTC) #38
Patrick McCarty
On 2010/08/28 19:16:18, Ian Hulin wrote: > > Patrick and Neil, do I have to ...
8 years, 10 months ago (2010-08-29 01:53:29 UTC) #39
hanwenn
On Sat, Aug 28, 2010 at 4:16 PM, Ian Hulin <ian@hulin.org.uk> wrote: > (define (make-music-type-predicate ...
8 years, 10 months ago (2010-08-30 02:31:36 UTC) #40
Ian Hulin (gmail)
Have new patch-set ready, Message describing this patch set: Can handler and stray dots from ...
8 years, 9 months ago (2010-08-31 17:31:49 UTC) #41
Ian Hulin
8 years, 9 months ago (2010-08-31 17:54:44 UTC) #42
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Han-Wen and all,
I can't upload the patch-set to Rietveld, and I seem to have lost
ownership of the issue on Reitveld, so I can't close it and open a new
issue either.

On 30/08/10 03:31, Han-Wen Nienhuys wrote:
> On Sat, Aug 28, 2010 at 4:16 PM, Ian Hulin <ian@hulin.org.uk> wrote:
>> (define (make-music-type-predicate . music-types)
>>  (define ((make-music-type-predicate-aux mtypes) expr)
>>    (if (null? mtypes)
>>        #f
>>        (or (eqv? (car mtypes) (ly:music-property expr 'name))
>>            ((make-music-type-predicate-aux (cdr mtypes)) expr))))
>>  (make-music-type-predicate-aux music-types))
>>
>> I'm a bit puzzled by this code.  Even if we move the nested definition
>> out from within make-music-predicate, how does it work?  Variable
>> make-music-type-predicate is not declared anywhere else, and yet here we
>> appear to ask guile to resolve and call it before it is declared and
>> bound to anything?
> 
> are you sure? I only see calls to the -aux helper.
> 
I'd forgotten about currying functions. Neil picked up about the need to
use a new module in 1.9.  I've found a way of conditionally using it if
we're running with guile V1.9+.

>> Trying this in guile V1.8.7 repl with a dummy for ly:music-property and
>> it will work as written, but in V1.9.11 it always fails with
>> ERROR: in procedure macro-expand:
>> ERROR: source expression failed to match any pattern in (define
>> ((make-music-type-predicate-aux mtypes) expr)  (if (null? mtypes) #f (or
>> (eqv? (car mtypes) (ly:music-property expr 'name))
>> ((make-music-type-predicate-aux (cdr mtypes)) expr))))
>>
>> V1.9.11 throws the error whether or not the repl is called with
>> - --no-auto-compile.  It also doesn't help to try and un-nest the inner
>> (define and declare it at top-level.
> 
> Where is the latest version of this patch?  I downloaded a version
> from rietveld which required the following patch to get to
> display-lily.scm
> 
Stray dots removed in attached path - along with handler in
ergonomic-simple-format.
> commit 57992da984aaf16d6161dc44e5d9f7cb290ac813
> Author: Han-Wen Nienhuys <hanwen@lilypond.org>
> Date:   Sun Aug 29 23:30:55 2010 -0300
> 
>     Remove stray dots.
> 
> diff --git a/scm/lily.scm b/scm/lily.scm
> index 0aa2ad4..f898962 100644
> --- a/scm/lily.scm
> +++ b/scm/lily.scm
> @@ -213,9 +213,9 @@ messages into errors.")
>    format)
> 
>  (define (simple-format-handler dest . rest)
> -    (if (string? dest)
> -        (apply fancy-format (cons #f (cons dest . rest)))
> -        (apply fancy-format (cons dest . rest))))
> +  (if (string? dest)
> +      (apply fancy-format (cons #f (cons dest rest)))
> +      (apply fancy-format (cons dest rest))))
> 

>> Patrick and Neil, do I have to fix all the compatibility problems in all
>> the scm files loaded by lily.scm in order to push what we have so far?
> 
> As long as the compile for 1.8 keeps working, I don't think it's a
> problem incrementally tackling 1.9  compatibility problems.
> 
Cheers,
Ian



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJMfUHYAAoJEBqidDirZqASIM8IAIgvhpveVxuj4OpHEom4xSpX
xDe2zhrn575cC8dfg876RMET9Swa54q/6EAG2AxH9nSEynJOS5VEFMCzRrwkisfS
fFrpmguluydqZ6FDjc4q/Ia1h0GGiH0CHWSMRow2WhVILMQtBKBnQBhRkYfU265X
ZwTNINt4/alFnjhnO9oJv9qnbQbLWL15g6hwzRvOLBbrOVONeVMl3TQF/tbt00Po
DPngHhizsqguA7YkG8fgNaylo/5MbHphG+yLzkkkzm97I8DSlpvKhkfuGbSZU7p3
ILzp/yQjEZ7gqyuZY6uSyc8sbwSQN0HHq7zrBEchUI3fa8y51MvTvudKWy27c3U=
=cnFp
-----END PGP SIGNATURE-----
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b