Re: Embed LilyPond source files inside generated PDF (issue 225040043 by address@hidden)

[dak]

https://codereview.appspot.com/225040043/diff/40001/lily/include/sources.hh
File lily/include/sources.hh (right):

https://codereview.appspot.com/225040043/diff/40001/lily/include/sources.hh#newcode27
lily/include/sources.hh:27: static SCM source_file_list;
static?  That means that every compilation unit including this header
file gets a private copy of source_file_list.  Hardly a good idea.

https://codereview.appspot.com/225040043/diff/40001/lily/sources.cc
File lily/sources.cc (right):

https://codereview.appspot.com/225040043/diff/40001/lily/sources.cc#newcode78
lily/sources.cc:78: source_file_list = scm_cons (ly_string2scm
(file_string), source_file_list);
source_file_list is a global variable here (and I might add that you
forget to protect it from garbage collection, likely inducing crashes,
but that's a different problem).  So if I do

lilypond -fembed-source-files a.ly b.ly

the PDF for b.ly will include the source(s) for a.ly.  That's
undesirable.  Is there any reason you don't rely on the per-parser array
source_files_ for your list of source files?

https://codereview.appspot.com/225040043/diff/40001/scm/backend-library.scm
File scm/backend-library.scm (right):

https://codereview.appspot.com/225040043/diff/40001/scm/backend-library.scm#newcode85
scm/backend-library.scm:85: "-P"
I read in "man gs"

       -P     Makes Ghostscript to look first in  the  current
directory  for
              library  files.   By default, Ghostscript no longer looks
in the
              current directory, unless, of course, the first explicitly
 sup‐
              plied directory is "." in -I.  See also the INITIALIZATION
FILES
              section below, and bundled Use.htm for  detailed
discussion  on
              search paths and how Ghostcript finds files.

This seems quite unrelated to the issue of this patch and the rationale
for specifying this option is totally undocumented.

What is this for, and should this not rather be a separate patch if
necessary for some reason for newer versions of GhostScript?

https://codereview.appspot.com/225040043/diff/40001/scm/framework-ps.scm
File scm/framework-ps.scm (right):

https://codereview.appspot.com/225040043/diff/40001/scm/framework-ps.scm#newcode421
scm/framework-ps.scm:421: (map (lambda (fname)
Use of `map' instead of `for-each'.

https://codereview.appspot.com/225040043/diff/40001/scm/framework-ps.scm#newcode422
scm/framework-ps.scm:422: (set! idx (1+ idx))
quite unschemish.  It seems cleaner to just give two list arguments to
for-each, the second being (iota (length source-list)).

Either that, or use a named let for your loop.

https://codereview.appspot.com/225040043/diff/40001/scm/framework-ps.scm#newcode425
scm/framework-ps.scm:425: [ /_objdef {ly~a_stream} /type /stream /OBJ
pdfmark
[ ... pdfmark is a really ugly pairing.  I'd rather use mark ... pdfmark

https://codereview.appspot.com/225040043/