Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

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

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

Patrick McCarty
Hi Ian,

Please see my new comment regarding this patch (below).

Thanks,
Patrick


http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
File scm/display-lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode34
scm/display-lily.scm:34:
Jan recently removed all of the curried definitions that were affected
by this conditional (use-modules ...) call.

I just compiled LilyPond (and the patch queue from my "guile" branch)
against Guile 2.0 *without* this part of your patch, and I didn't run
into any issues.

In other words, you can safely remove this chunk from your patch.

http://codereview.appspot.com/2219044/

---
----
Join the Frogs!

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

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

Patrick McCarty

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
File scm/display-lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode34
scm/display-lily.scm:34:
On 2011/02/17 15:07:00, ianhulin44 wrote:
> In which case, do we even need lily.scm to pull in (ice-9
curried-definitions)
> at all if we aren't currying in our code?

We are in musicxml2ly and a few snippets:

$ git grep -l '(define (('
Documentation/music-glossary.tely
Documentation/snippets/compound-time-signatures.ly
Documentation/snippets/heavily-customized-polymetric-time-signatures.ly
Documentation/snippets/new/compound-time-signatures.ly
scripts/musicxml2ly.py

http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode308
scm/display-lily.scm:308:
Keep this chunk, because we want to use ly:load (even if the semantics
of ly:load have to change).

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm#newcode222
scm/lily.scm:222: (cond
On 2011/02/17 15:07:00, ianhulin44 wrote:
> This block is the whole point of the patch and the tracker.
> Jan has just re-written code in display-lily.scm so it doesn't curry.
If
> there's no currying in Lily Scheme code do we need this, or should we
defend
> against users using currying their Scheme code?

> Opinions please?

See my comment above.  I think we should keep it.

Since we're still supporting Guile 1.8 and 2.0 simultaneously, and Guile
1.8 supports currying out of the box, IMO it would not be smart to start
discouraging it.

Once everyone is using Guile 2.0 and people realize it doesn't support
currying out of the box, then I think people will naturally stop using
currying.  Even then, I don't think we would need to actively discourage
it.

http://codereview.appspot.com/2219044/diff/25001/scm/lily.scm#newcode291
scm/lily.scm:291: (primitive-load-path file-name)  ;; to support Guile
V2 autocompile
On 2011/02/17 15:07:00, ianhulin44 wrote:
> When we move to generating our own compiled Scheme files ly:load will
need a
> significant re-write. We will also need routines to do the compilation
and some
> extra  changes in the Guile initialisation code  This change makes no
difference
> using Guile V1.8 but is only temporary debug code until Tracker 1349
is fixed,
> and the code to support compiling out scheme files to scm/out.

Yes, but without this change, SCM files are not autocompiled.  Is this
still the case?

http://codereview.appspot.com/2219044/

---
----
Join the Frogs!

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

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

Patrick McCarty
In reply to this post by Patrick McCarty
On 2011/02/17 17:05:25, Reinhold wrote:
> http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm
> File scm/display-lily.scm (right):


http://codereview.appspot.com/2219044/diff/25001/scm/display-lily.scm#newcode34
> scm/display-lily.scm:34:

> > We are in musicxml2ly and a few snippets:
> >
> > $ git grep -l '(define (('

> Is define-public relevant, too? There are lots of hits for
"(define-public (("
> in our scm/ directory...

Yes, I thought of that too after sending my email.  :)

Thanks,
Patrick

http://codereview.appspot.com/2219044/

---
----
Join the Frogs!

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

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

Patrick McCarty
In reply to this post by Patrick McCarty
LGTM.

Can you email me your patch so I can apply it?

Thanks,
Patrick

http://codereview.appspot.com/2219044/

---
----
Join the Frogs!

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

Re: T1247 - Conditionally do (use-modules (ice-9 curried-definitions)) if running with Guile V2, (issue2219044)

Patrick McCarty
On Sun, Feb 20, 2011 at 4:38 AM, Ian Hulin <[hidden email]> wrote:
> Hi Patrick,
>
> On 18/02/11 02:13, [hidden email] wrote:
>> LGTM.
>>
>> Can you email me your patch so I can apply it?
>
> Here's the patch.

I retested everything, and the patch checks out just fine.  I've pushed it.

Thanks,
Patrick

---
----
Join the Frogs!

Loading...