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

classic Classic list List threaded Threaded
4 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)

Carl.D.Sorensen
I like the code for choosing Guile V2.

I'm concerned about a bunch of the re-indentation changes.

Thanks,

Carl



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

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode197
scm/lily.scm:197: ;(set-debug-cell-accesses! 1000)
Why is this shoved over to the right?

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode199
scm/lily.scm:199: ;;; Boolean thunk - are we integrating Guile V2.0 or
higher with Lilypond?
It should either be LilyPond (the application) or lilypond (the
executable).

I don't think it should ever be Lilypond.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode204
scm/lily.scm:204: (ice-9 safe)
These should all be aligned with (ice-9 regex) as in the old code.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode300
scm/lily.scm:300: (vector-ref (uname) 0) char-set:letter+digit)))
(vector-ref should be indented; it's an argument to string-tokenize.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode337
scm/lily.scm:337: iface))
Looks like there are trailing spaces here?

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode568
scm/lily.scm:568: r5rs-secondary-predicates
Why the move from spaces to tabs?  As far as I know, the tabs-vs-spaces
discussion is not settled.  Given that it's not, I don't think we should
be changing existing code when that's the only change.

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode829
scm/lily.scm:829: (ly:stderr-redirect (format "~a.log" base) "w"))
The previous indentation here was correct.  Then clause should be
aligned with if clause

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode831
scm/lily.scm:831: (format ping-log "Processing ~a\n" base))
Same here

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode848
scm/lily.scm:848: (ly:reset-all-fonts))))
Else clause should be aligned with if clause.

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)

Neil Puttock

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

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode226
scm/lily.scm:226: (ly:progress  (_ "Using (ice-9 curried-definitions)
module\n"))
   (if (ly:get-option 'verbose)
       (ly:message (_ "Using (ice-9 curried-definitions) module")))
   (use-modules (ice-9 curried-definitions)))

http://codereview.appspot.com/2219044/diff/1/scm/lily.scm#newcode231
scm/lily.scm:231: (_ "module (ice-9 curried-definitions) not in Guile
1.8\n")))))
I don't think ths message is necessary.

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)

Neil Puttock
In reply to this post by Carl.D.Sorensen
On 2010/09/25 23:39:33, ianhulin44 wrote:

> Looking at warn.cc I can see ly:progress and ly:message are
synonymous.

ly:message always starts on a new line.

ly:progress calls progress_indication () directly, which has this
comment above it:

> I felt some indication was useful at run-time that we weren't trying
to use the
> V2-specific modules when running with --verbose using Guile V1.8.

Why is that useful?  It's irrelevant for anybody using 1.8.

Cheers,
Neil

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 Carl.D.Sorensen
On 2010/09/26 10:19:59, ianhulin44 wrote:
> On 2010/09/26 00:00:27, Neil Puttock wrote:
> >
> > Why is that useful?  It's irrelevant for anybody using 1.8.

> --verbose messages are used for development and maintenance purposes
as well as
> for advanced user hacking.

Yes.

> E.g. messages are output for each file loaded by lily,scm when it's
run up
> during initialization.
> Files loaded like this are dynamically byte-compiled by Guile V2.

Are they?  Last time I tested this, Guile only compiled modules loaded
via (use-modules ...).

I had to change `primitive-load' to `primitive-load-path' (in ly:load)
to enable automatic compilation.

> Guile V2 also dynamically compiles any files accessed via
(use-modules(...))
> both by lily.scm and any .scm files loaded by lily.scm: it's a PITA
keeping
> track of all this.

Yes, but only while you debug this particular issue (see below)...

> This patch adds a new module solely for use by Guile V2.0.  I have
work coming
> up (T1249) which implies we will need to use modules under V1.8 which
will be
> part of the Guile V2.0 base code, i.e. we will need to conditionally
load them
> when running with Guile V1.8 and not V2 (e.g. (ice-9 syncase) so we
can fix a V2
> compilation problem in ly-syntax-contructors.scm).
> At the moment, I have a use for such messages while working on the
Guile
> migration stuff.  If you feel they are a serious nuisance for users
running V1.8
> and using --verbose by all means open an issue and I'll take them out
once all
> the issues spawned by T1055 are fixed and LilyPond is running up
without
> problems using both Guile V1.8 and V2.

Okay, I agree that things are getting complicated quite fast...

But will there really be a use for this ly:message call *after* your
patch is committed to git?

-Patrick

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

---
----
Join the Frogs!

Loading...