fix ly:parser-parse-file in an ly file

Replace the use of parser-parse-string with a new interface ly:parser-include-string.

Benjamin recommends that ly:parser-parse-file should be banned from .ly files.

[Carl, 2010-06-03 18:00:16]

Benjamin has sent a revised patch that addresses Neil's concerns.

Please review the patch.

THanks,

Carl

[benjamin_python.org, 2010-06-03 20:11:36]

2010/6/3  <Carl.D.Sorensen@gmail.com>:
> Reviewers: ,
>
> Message:
> Benjamin has sent a revised patch that addresses Neil's concerns.
>
> Please review the patch.
>
> THanks,
>
> Carl
>
>
> Description:
> fix ly:parser-parse-file in an ly file
> This switches to using the flex buffer stack.

Note that this description isn't correct anymore. This patch doesn't
actually prevent those functions from segfaulting but adds a new
interface ly:parser-include-string. I think ly:parser-parse-string and
ly:parser-parse-file should actually be banned in .ly files.



-- 
Regards,
Benjamin

[Carl, 2010-06-03 20:48:29]

Thanks,  I've changed the description.

[dak, 2010-06-04 15:57:43]

> Note that this description isn't correct anymore. This patch doesn't
> actually prevent those functions from segfaulting but adds a new
> interface ly:parser-include-string. I think ly:parser-parse-string and
> ly:parser-parse-file should actually be banned in .ly files.

If ly:parser-include-string offers all the functionality of
ly:parser-parse-string and ly:parser-parse-file, the latter should be
implemented in terms of the former (rather than segfaulting in some situations)
for compatibility.

If it doesn't offer all functionality, banning the old functions without
offering a substitute seems like a bad thing to do to the user.

[benjamin_python.org, 2010-06-04 16:14:39]

2010/6/4  <dak@gnu.org>:
>  > Note that this description isn't correct anymore. This patch doesn't
>>
>> actually prevent those functions from segfaulting but adds a new
>> interface ly:parser-include-string. I think ly:parser-parse-string and
>> ly:parser-parse-file should actually be banned in .ly files.
>
> If ly:parser-include-string offers all the functionality of
> ly:parser-parse-string and ly:parser-parse-file, the latter should be
> implemented in terms of the former (rather than segfaulting in some
> situations) for compatibility.

ly:parser-parse-string and ly:parser-parse-file try to run the whole
translation tool chain, which as far as I can tell is not a use case.
I think it is appropriate to add a new interface because it does
something different and more useful.



-- 
Regards,
Benjamin

[Reinhold, 2010-06-07 13:59:45]

On 2010/06/04 16:14:39, benjamin_python.org wrote:
> ly:parser-parse-string and ly:parser-parse-file try to run the whole
> translation tool chain, which as far as I can tell is not a use case.
> I think it is appropriate to add a new interface because it does
> something different and more useful.

Where would the old ly:parser-parse-string ever be needed? so far, I have only
seen it used as e.g.
     (ly:parser-parse-string
      (ly:parser-clone parser)
      (format #f "\\include \"~a\""
              (include-pathname "score"))))
which would work even better with the new system, right? 

The only occurrence where it might make sense to have a separate parser is in
scm/parser-ly-from-scheme.scm in the function read-lily-expression, which calls
parse-string-result. However, I fail to see where this is actually used...

[Neil Puttock, 2010-06-07 19:17:27]

On 2010/06/07 13:59:45, Reinhold wrote:

> The only occurrence where it might make sense to have a separate parser is in
> scm/parser-ly-from-scheme.scm in the function read-lily-expression, which
calls
> parse-string-result. However, I fail to see where this is actually used...

It's called whenever Guile encounters the character sequence #{ for parsing
LilyPond expressions inside scheme (i.e., inside music functions).

[Reinhold, 2010-06-07 19:35:49]

On 2010/06/07 19:17:27, Neil Puttock wrote:
> On 2010/06/07 13:59:45, Reinhold wrote:
> > However, I fail to see where this is actually used...
> 
> It's called whenever Guile encounters the character sequence #{ for parsing
> LilyPond expressions inside scheme (i.e., inside music functions).

Ah, thanks for enlightening me! In this case, I suppose we really want a copy of
the parser, since we make available the function's variables as $varname...

[nicolas.sceaux, 2010-06-08 19:08:39]

Le 7 juin 2010 à 21:35, reinhold.kainhofer@gmail.com a écrit :

> On 2010/06/07 19:17:27, Neil Puttock wrote:
>> On 2010/06/07 13:59:45, Reinhold wrote:
>> > However, I fail to see where this is actually used...
> 
>> It's called whenever Guile encounters the character sequence #{ for
> parsing
>> LilyPond expressions inside scheme (i.e., inside music functions).
> 
> Ah, thanks for enlightening me! In this case, I suppose we really want a
> copy of the parser, since we make available the function's variables as
> $varname...

Not exactly: when $varname is encountered, it is replaced by a generated
identifier (lilyvartmp + some characters: a, b, c, ..., aa, ab, etc)

[Carl, 2010-07-02 15:55:27]

I'm not sure where we are on this patch; we've had some philosophical discussion
whose end I'm not sure I understand.

Is this OK to push?

[Neil Puttock, 2010-07-08 23:47:34]

On 2010/07/02 15:55:27, Carl wrote:
> I'm not sure where we are on this patch; we've had some philosophical
discussion
> whose end I'm not sure I understand.
> 
> Is this OK to push?

LGTM, save a few style nitpicks.

[Carl, 2010-07-11 04:08:30]

Neil has approved.  He mentioned some style nitpicks.  I went through looking
for style nitpicks, and found two.

If we get these cleared up, then I'll push.

http://codereview.appspot.com/1345041/diff/2001/3004
File lily/include/lily-parser.hh (right):

http://codereview.appspot.com/1345041/diff/2001/3004#newcode68
lily/include/lily-parser.hh:68: void include_string (string ly_code);
Should be alphabetic -- before parse_file

http://codereview.appspot.com/1345041/diff/2001/3005
File lily/lexer.ll (right):

http://codereview.appspot.com/1345041/diff/2001/3005#newcode371
lily/lexer.ll:371: for (size_t i = 0; i < pending_string_includes_.size(); i++)
{
{ should be on next line, although we're not consistent in this file.  But I
think it's the desired structure.

[Valentin Villenave, 2010-07-18 09:08:19]

On 2010/07/11 04:08:30, Carl wrote:
> If we get these cleared up, then I'll push.

Hello guys,

if you don't mind me asking, where are we wrt this patch?

If it only comes down to a coding style problem, I'm pretty sure I've already
seen worse in existing source code :)

Cheers,
Valentin

[Neil Puttock, 2010-07-18 10:13:58]

On 2010/07/18 09:08:19, Valentin Villenave wrote:

> if you don't mind me asking, where are we wrt this patch?
> 
> If it only comes down to a coding style problem, I'm pretty sure I've already
> seen worse in existing source code :)

I've prepared a patch which I'll push later.

Cheers,
Neil