Re: PEG Parser

[Michael Lucy]

Hi,

I'm the guy that originally wrote this for GSOC, so I figured I'd jump
in.  I'd be happy to help with getting the PEG module merge-ready.

On Wed, Jan 26, 2011 at 7:40 PM, Noah Lavine <address@hidden> wrote:
> Hello again,
>
> I've attached my coverage results. The html file expects the css file
> to be in the same directory. If you look at the html file, you'll see
> that almost all of peg.scm is hit by the tests.
>
> As far as I can tell, the two functions that are not tested are
> keyword-flatten (line 512) and peg-string-compile (line 713). I looked
> at these, but I don't understand what either of them does well enough
> to test them yet.

keyword-flatten is described in api-peg.texi.  It's basically a
special case of context-flatten which collapses S-expressions
according to the symbol they start with.  From the documentation:
@deffn {Scheme Procedure} keyword-flatten terms lst
A less general form of @code{context-flatten}. Takes a list of
terminal atoms @code{terms} and flattens @var{lst} until all elements
are either atoms, or lists which have an atom from @code{terms} as
their first element.
@lisp
(keyword-flatten '(a b) '(c a b (a c) (b c) (c (b a) (c a)))) @result{}
(c a b (a c) (b c) c (b a) c a)
@end lisp

peg-string-compile is a function that will compile PEGs expressed as
strings into lambda expressions.  It does this by first parsing the
string using the PEG-parsing-PEG, then turning the output into an
S-expression representation of a PEG, compressing it, and passing the
compressed S-expression to peg-sexp-compile.  It's used if you e.g.
call peg-match with a string instead of an S-expression as the first
argument.

>
> Other than that, some error code is not hit, and the ends of some cond
> clauses. These should be tested more, but I need to understand the
> code more to know what will test them. There are also a few lines that
> I find suspicious, in particular lines 39, 134-136, 146, 157, 165,
> 506-508, and 649. (Ludovic - sorry I haven't isolated test cases. I'm
> just pointing these out now to show that possibly the test suite tests
> more than the coverage makes it appear. In the future I might be able
> to isolate the issues.)
>
> Lines 14-15 look to me like a function that was used for debugging and
> now serves no purpose.
>
> Let me give a few more overall thoughts on peg.scm, after working with
> it for a few more days. It looks like good code, but the documentation
> isn't great. It took me several read-throughs to figure out what some
> of it did, and I'm still not sure about those two functions that don't
> have tests. (Although they are a small part of the overall module.)
> I'm not sure what this means about its fitness to merge.

If you give me some more details on the parts that are weakest I'll
try and beef up the documentation.

>
> Noah
>
> On Sun, Jan 23, 2011 at 8:29 PM, Noah Lavine <address@hidden> wrote:
>> Hello all,
>>
>>> It should have produced $top_builddir/guile.info, which can be used as
>>> input to LCOV to generate an HTML code coverage report
>>> (http://ltp.sourceforge.net/coverage/lcov.php).
>>
>> Oh, that worked. The current tests check 92.6% of the lines in
>> peg.scm, and 90.7% of the functions. I looked through lcov's HTML
>> guide, and it looks like what the tests miss is almost all
>> error-handling code. However, I must say that the HTML output looked a
>> bit suspicious - for instance, there were places where the first line
>> of a function was marked as hit, but the second line was not.
>>
>> On another note, I looked at the PEG documentation, and it was quite good.
>>
>> When I merged the 'mlucy' branch into Guile mainline, the merge went
>> almost cleanly - the only issues were a page of links in Guile's
>> documentation, which was a two-line issue, and ice-9/psyntax-pp.scm,
>> which I fixed by choosing the mainline's version and had no problems.
>>
>> Given this, what are the issues blocking PEG being merged (if there
>> are any)? I'd like to work on them.
>>
>> Noah
>>
>