Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

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 wrote:
> On 2010/05/13 20:37:23, Ian Hulin wrote:
> > On 2010/05/12 02:24:19, hanwenn wrote:
> > > move to use, all of them
> >
> > Sorry Han-Wen, I don't understand your feedback, could you clarify?


> rather than

>    SCM x = ..;
>    SCM y = ..;

>    z = foo(x);
>    call(y);

> do


>    SCM x = ..;
>    z = foo(x);

>    SCM y = ..;
>    call(y);

> i.e. declare vars just before their first use.

I really don't like interlacing data declarations with executable code,
and I'd have felt happier if all the declarations were at the top of the
function separate from the code. . .
I've coded it as I have so only the relevant changes show up, even
though I personally disagree with the existing coding style.
We obviously come from opposing schools of programming style so if it
doesn't affect the functionality we'll have to agree to differ on this
one.

http://www.codereview.appspot.com/1160044/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Patrick McCarty
On 2010/05/15 19:56:47, Ian Hulin wrote:

> On 2010/05/14 03:23:55, hanwenn wrote:
> >
> > rather than
> >
> >   SCM x = ..;
> >   SCM y = ..;
> >
> >   z = foo(x);
> >   call(y);
> >
> > do
> >
> >
> >   SCM x = ..;
> >   z = foo(x);
> >
> >   SCM y = ..;
> >   call(y);
> >
> > i.e. declare vars just before their first use.

> I really don't like interlacing data declarations with
> executable code, and I'd have felt happier if all the
> declarations were at the top of the function separate from the
> code. . .  I've coded it as I have so only the relevant changes
> show up, even though I personally disagree with the existing
> coding style.  We obviously come from opposing schools of
> programming style so if it doesn't affect the functionality
> we'll have to agree to differ on this one.

Hi Ian,

If the location at which a variable is declared/initialized is so
close to its place of usage, why bother putting the declarations
at the top of a block?

IMO, it's not worth going through the trouble of separating
everything unless it's necessary.  From my experience, it
decreases code readability.  I would do it like this:

   SCM maker = ly_lily_module_constant ("make-module");
   mod = scm_call_0 (maker);

   SCM module_export_all_x = ly_lily_module_constant
("module-export-all!");
   scm_call_1 (module_export_all_x, mod);

   SCM scm_module = ly_lily_module_constant ("the-scm-module");
   ly_use_module (mod, scm_module);
   ly_use_module (mod, global_lily_module);


http://www.codereview.appspot.com/1160044/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Patrick McCarty
In reply to this post by Ian Hulin
Hi Ian,

I haven't tested your patch yet to make sure everything is okay, but I
have a few quick comments for you.

Thanks,
Patrick


http://www.codereview.appspot.com/1160044/diff/10001/11008
File scm/lily.scm (right):

http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode250
scm/lily.scm:250: (define-public  (guile-v2 )
(define-public (guile-v2)

http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode251
scm/lily.scm:251: (string>? (version) "1.8.7"))
What if the Guile team decides to release 1.8.8 ?

It would be safer to change this to some 1.9 version...

http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode303
scm/lily.scm:303: (cond-expand
Shouldn't this `cond-expand' block start in column 1, and then indented
from there?

http://www.codereview.appspot.com/1160044/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Patrick McCarty
In reply to this post by 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:
> What if the Guile team decides to release 1.8.8 ?

> It would be safer to change this to some 1.9 version...

Oh yes, and please fix the indentation here too.

Which editor are you using that uses 4 spaces for indentation of Scheme
code?

http://www.codereview.appspot.com/1160044/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
In reply to this post by Ian Hulin
On 2010/05/15 21:39:39, Patrick McCarty wrote:

> On 2010/05/15 19:56:47, Ian Hulin wrote:
> > On 2010/05/14 03:23:55, hanwenn wrote:
> > >
> > > rather than
> > >
> > >   SCM x = ..;
> > >   SCM y = ..;
> > >
> > >   z = foo(x);
> > >   call(y);
> > >
> > > do
> > >
> > >
> > >   SCM x = ..;
> > >   z = foo(x);
> > >
> > >   SCM y = ..;
> > >   call(y);
> > >
> > > i.e. declare vars just before their first use.
> >
> > I really don't like interlacing data declarations with
> > executable code, and I'd have felt happier if all the
> > declarations were at the top of the function separate from the
> > code. . .  I've coded it as I have so only the relevant changes
> > show up, even though I personally disagree with the existing
> > coding style.  We obviously come from opposing schools of
> > programming style so if it doesn't affect the functionality
> > we'll have to agree to differ on this one.

> Hi Ian,

> If the location at which a variable is declared/initialized is so
> close to its place of usage, why bother putting the declarations
> at the top of a block?

> IMO, it's not worth going through the trouble of separating
> everything unless it's necessary.  From my experience, it
> decreases code readability.  I would do it like this:

>    SCM maker = ly_lily_module_constant ("make-module");
>    mod = scm_call_0 (maker);

>    SCM module_export_all_x = ly_lily_module_constant
("module-export-all!");
>    scm_call_1 (module_export_all_x, mod);

>    SCM scm_module = ly_lily_module_constant ("the-scm-module");
>    ly_use_module (mod, scm_module);
>    ly_use_module (mod, global_lily_module);

I'll reluctantly conform if that's the house style, but it needs to be
documented as a standard in the Contributors Guide.

I've added the comments so hopefully another maintainer would find it
simple to see what's going on without having to bother Han-Wen or the
developers' list.

Cheers,
Ian

http://www.codereview.appspot.com/1160044/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
In reply to this post by 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:
> What if the Guile team decides to release 1.8.8 ?

> It would be safer to change this to some 1.9 version...
Changed to "1.9.0"
... and fixed indentation

http://www.codereview.appspot.com/1160044/diff/10001/11008#newcode303
scm/lily.scm:303: (cond-expand
On 2010/05/15 21:53:15, Patrick McCarty wrote:
> Shouldn't this `cond-expand' block start in column 1, and then
indented from
> there?

Done.

http://www.codereview.appspot.com/1160044/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Neil Puttock
In reply to this post by Ian Hulin

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) "1.9.0"))
This must at least be "1.9.10".

http://www.codereview.appspot.com/1160044/diff/20001/16009#newcode304
scm/lily.scm:304: ((not guile-2)
guile-v2

otherwise it's always expanded

http://www.codereview.appspot.com/1160044/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
In reply to this post by 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) "1.9.0"))
On 2010/05/17 22:44:22, Neil Puttock wrote:
> This must at least be "1.9.10".

Done.

http://www.codereview.appspot.com/1160044/diff/20001/16009#newcode304
scm/lily.scm:304: ((not guile-2)
On 2010/05/17 22:44:22, Neil Puttock wrote:
> guile-v2

> otherwise it's always expanded

Ouch! Nice catch.

http://www.codereview.appspot.com/1160044/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Neil Puttock
In reply to this post by Ian Hulin
Hi Ian,

I've tried testing your latest patch, but it fails on clip-systems.ly.

`make-rhythmic-location' (defined in clip-region.scm) appears to be
inaccessible from a \layout block.

Cheers,
Neil

http://www.codereview.appspot.com/1160044/show

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
Hi Neil,

On 19/05/10 23:42, [hidden email] wrote:
> Hi Ian,
>
> I've tried testing your latest patch, but it fails on clip-systems.ly.
>
> `make-rhythmic-location' (defined in clip-region.scm) appears to be
> inaccessible from a \layout block.
Rats. . .
I'll investigate but I may need some help with this.

Cheers,

Ian

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
In reply to this post by Neil Puttock
On 19/05/10 23:42, [hidden email] wrote:
Hi Ian,

I've tried testing your latest patch, but it fails on clip-systems.ly.

`make-rhythmic-location' (defined in clip-region.scm) appears to be
inaccessible from a \layout block.

Cheers,
Neil

http://www.codereview.appspot.com/1160044/show

______________________________________________       This email has been scanned by Netintelligence       http://www.netintelligence.com/email


Hi all,

I've been having a look at this, and I need a bit more info, Han-Wenn, on why the C++ code uses the %module-public-interface the way it does

SCM mod = SCM_EOL;
  if (!safe)
    {
      SCM maker = ly_lily_module_constant ("make-module");

      SCM scm_module = ly_lily_module_constant ("the-scm-module");

      mod = scm_call_0 (maker);
      scm_module_define (mod, ly_symbol2scm ("%module-public-interface"),
             mod);

      ly_use_module (mod, scm_module);
      ly_use_module (mod, global_lily_module);
    }
  else
    {
// snip . . .
    }

Which is sort of like doing this in Scheme:
(define mod (make-module))
(define curmod '())
(define scm_module (resolve-module '(the-scm-module)))
(define global-lily-module '(lily))
(module-define! mod %module-public-interface mod)
(set! curmod (set-current-module mod))
(use-modules (scm-module global-lily-module))
(set-current-module curmod)

(please excuse the bad Scheme).

Also, ly_use_module does:
SCM
ly_use_module (SCM mod, SCM used)
{
  SCM expr
    = scm_list_3 (ly_symbol2scm ("module-use!"),
          mod,
          scm_list_2 (ly_symbol2scm ("module-public-interface"),
/*                                                    ^no % here                 */
                  used));

  return scm_eval (expr, global_lily_module);
}

Han-Wenn, What's the difference between the two ly_symbol2scm calls?

This is from a discussion on Lily's use of %module-public-interface on guile-devel

"
Actually the trick wouldn’t work in cases where the ‘%module-public-interface’ binding is mutated, as with Lilypond."
 
Ludovic, what did you mean by this?  Please explain in terms a scheme-coding simpleton (i.e. me) would understand.  Why is what the Lilypond code is doing such A Bad Thing as far as guile is concerned?   How could we re-code to achieve the same results in a way that guile supports?  It looks like one of Lilypond's regression tests really depends on this and the replacement scheme code Andy Wingo gave us (module-export-all!) doesn't emulate this behaviour well enough to pass the Lilypond regression test.

I'm a part-time developer and I feel I'm in way over my head on this now.

If anyone can throw a life-raft to me on this one before I sink for the third time in the sea of incomprehension it would be appreciated.

Glug, glug,  glug . . .

Cheers,

Ian

 

 
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Patrick McCarty
On Thu, Jul 15, 2010 at 4:30 PM, Ian Hulin <[hidden email]> wrote:

>
> I've been having a look at this, and I need a bit more info, Han-Wenn, on
> why the C++ code uses the %module-public-interface the way it does
>
> SCM mod = SCM_EOL;
>   if (!safe)
>     {
>       SCM maker = ly_lily_module_constant ("make-module");
>
>       SCM scm_module = ly_lily_module_constant ("the-scm-module");
>
>       mod = scm_call_0 (maker);
>       scm_module_define (mod, ly_symbol2scm ("%module-public-interface"),
>              mod);
>
>       ly_use_module (mod, scm_module);
>       ly_use_module (mod, global_lily_module);
>     }
>   else
>     {
> // snip . . .
>     }
>
> Which is sort of like doing this in Scheme:
> (define mod (make-module))
> (define curmod '())
> (define scm_module (resolve-module '(the-scm-module)))
> (define global-lily-module '(lily))
> (module-define! mod %module-public-interface mod)
> (set! curmod (set-current-module mod))
> (use-modules (scm-module global-lily-module))
> (set-current-module curmod)
>
> (please excuse the bad Scheme).
>
> Also, ly_use_module does:
> SCM
> ly_use_module (SCM mod, SCM used)
> {
>   SCM expr
>     = scm_list_3 (ly_symbol2scm ("module-use!"),
>           mod,
>           scm_list_2 (ly_symbol2scm ("module-public-interface"),
> /*                                                    ^no %
> here                 */
>                   used));
>
>   return scm_eval (expr, global_lily_module);
> }
>
> Han-Wenn, What's the difference between the two ly_symbol2scm calls?
Hi Ian,

I'm glad you noticed that part from ly_use_module().  Whatever the
"module-public-interface" binding (without the %) was used for, it
seems to prevent modules from loading correctly.

Can you test the attached patch (with Guile 1.9) to see if it works
for you?  I haven't tested this with Guile 1.8 yet.

Thanks,
Patrick

test.patch (622 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Patrick,

On 27/07/10 00:05, Patrick McCarty wrote:

> On Thu, Jul 15, 2010 at 4:30 PM, Ian Hulin <[hidden email]> wrote:
>>
>> I've been having a look at this, and I need a bit more info, Han-Wenn, on
>> why the C++ code uses the %module-public-interface the way it does
>>
>> SCM mod = SCM_EOL;
>>   if (!safe)
>>     {
>>       SCM maker = ly_lily_module_constant ("make-module");
>>
>>       SCM scm_module = ly_lily_module_constant ("the-scm-module");
>>
>>       mod = scm_call_0 (maker);
>>       scm_module_define (mod, ly_symbol2scm ("%module-public-interface"),
>>              mod);
>>
>>       ly_use_module (mod, scm_module);
>>       ly_use_module (mod, global_lily_module);
>>     }
>>   else
>>     {
>> // snip . . .
>>     }
>>
>> Which is sort of like doing this in Scheme:
>> (define mod (make-module))
>> (define curmod '())
>> (define scm_module (resolve-module '(the-scm-module)))
>> (define global-lily-module '(lily))
>> (module-define! mod %module-public-interface mod)
>> (set! curmod (set-current-module mod))
>> (use-modules (scm-module global-lily-module))
>> (set-current-module curmod)
>>
>> (please excuse the bad Scheme).
>>
>> Also, ly_use_module does:
>> SCM
>> ly_use_module (SCM mod, SCM used)
>> {
>>   SCM expr
>>     = scm_list_3 (ly_symbol2scm ("module-use!"),
>>           mod,
>>           scm_list_2 (ly_symbol2scm ("module-public-interface"),
>> /*                                                    ^no %
>> here                 */
>>                   used));
>>
>>   return scm_eval (expr, global_lily_module);
>> }
>>
>> Han-Wenn, What's the difference between the two ly_symbol2scm calls?
>
> Hi Ian,
>
> I'm glad you noticed that part from ly_use_module().  Whatever the
> "module-public-interface" binding (without the %) was used for, it
> seems to prevent modules from loading correctly.
>
> Can you test the attached patch (with Guile 1.9) to see if it works
> for you?  I haven't tested this with Guile 1.8 yet.

I've tested with V1.8 and it's fine.  I've merged it into the patch for
Tracker-1055 which I am just regression-testing at the moment. Keep your
fingers crossed - it looks like it's got past the clip-regions test
which was failing before.  Looks like the ly_use_module line was the
problem.  Thanks, nice catch.

Cheers,
Ian

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

iQEcBAEBAgAGBQJMUFOTAAoJEBqidDirZqASPpUIANCek8vKApQxeNw1PIxYNc55
eOy6+lDR/qH7DBY/+kBUQdEIMR8sJfgrDe2oIoBvyxFGDg4qLSNLukbJuSovZQkh
Gqd+o5a0J8r6kZ4WoXwRLOo4dHahG6QSdaq0Zo5lTBK3eKYLLC5Um/x/V551QohX
58udpzYhVnDEFtEjV1ok11qTO3XjMnsO+UAp8PZFwSdJfeFn6S5Ot0z64UeeR33U
uhECR3MjiS4gdOGjyNoBBtfNYC8JfxwuuISAwdLFFpeLVTe+W3PMUH3mFNb4DOjg
ttVsHmq118gPgrCNZt1wVVDJ/75SsEdeaEGmNndMPmND++7BQI5N6/3c2gtvF8w=
=L1g0
-----END PGP SIGNATURE-----

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: T1055: Avoid using deprecated %module-public-interface in guile initialisation. (issue1160044)

Ian Hulin
In reply to this post by Neil Puttock
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi all,

On 19/05/10 23:42, [hidden email] wrote:

> Hi Ian,
>
> I've tried testing your latest patch, but it fails on clip-systems.ly.
>
> `make-rhythmic-location' (defined in clip-region.scm) appears to be
> inaccessible from a \layout block.
>
> Cheers,
> Neil
>

A new patch-set is available for review on Rietveld which fixes this
problem.

> http://www.codereview.appspot.com/1160044/show

Cheers,

Ian

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

iQEcBAEBAgAGBQJMULPbAAoJEBqidDirZqASEhwH/A414FW1yozKvhwhGFwTYPqB
KNRIFkZU5P77MxDLHhqZdn8tGkaVv7E0Qee+yShfW/P7g2prd/DW9loiHj0hucPS
VRHnQBe8QQ97d8aMsN9K+vXg9/p+JqP2mevzYGF6ZJIJHfXVBYOJlzfxjU8gUEWD
rIfqVqpw1gjoSZ0epCexz5Oac+saT6pzFa+kuJsJDnd5ZfuAbpWUBKU2X779AQIo
Kj7ny9swypkwgB+oTHkEF7+/t1goCy3+Y7CkL1eCsLhu4iN22YjfOfY4fw1ldhp5
z9KU0JUsC0NkbLL1EiQNzGmjFoz5srUd3wPIaoPo2cf4dQQXvZ6jmxUsK/3RPpU=
=RYS3
-----END PGP SIGNATURE-----

---
----
Join the Frogs!

Loading...