Thanks for the vast answers and pointing out critical parts of the implementation. On 2011-08-11, at 08:14, Tudor Girba wrote:

Hi, Camillo, thanks for taking an interest and for letting us know that you committed.

Lukas, thanks for the answer. I would also like to add one comment: omit is maybe interesting for small and quick parsers in which you have the grammar and the parser together. However, for anything significant, if you want to split the grammar and the various parsers, it all of a sudden becomes less useful because you do not want to restrict the grammar.

Yeah and that's exactly the reason why I think this is useful. Since we want to use PetitParser also for small "dirty" projects where portability is a secondary problem. And I still disagree that it becomes less useful, it is just a way of filtering your output from parser in a very simple way ;). Furthermore having the omit doesn't mean that you will have to use it. t mean there is also the #flatten message which goes somewhat into the same direction IMO, since it is there for modifying directly in the parser the data. It could be omitted as well and put into the grammar.

I also took a quick look at the implementation. There are simply too many ifs that add complexity to an already non-trivial code.

Indeed the implementation is not very nice, and more of a preliminary hack to see if it works, I will definitely do a second pass with lukas' suggestion for a first-class object to denote the omit property.

All in all, I think we should retire this add-on.

Cheers, Doru

On 11 Aug 2011, at 07:31, Lukas Renggli wrote:

...

Hi Camillo,

...

I quickly hacked (with a lot of tests) support for omit in PetitParser. This should simplify some parsing efforts as an additional filtering step could be avoided.

parser := ($" asParser omit, $" asParser negate star flatten, $" asParser omit) parser parse: '"asdfsd"' "yields directly #('asdfsd') "

vs

parser := ($" asParser, $" asParser negate star flatten, $" asParser) parser parse: '"asdfsd"'

which yields #($" 'asdfsd' $")

I don't really see use of #omit. The element is still in a collection and you need to extract it from there. What is the benefit other than it is now at index 1 instead of 2?

Also the implementation has some problems:

1. #omit makes it really hard to compose grammars, because parsers suddenly get very asymmetric and start to affect each others behavior depending on how you compose them.

it shouldn't be about the behavior of the grammes, I might have not done everything correctly in my implementation, but the way the parsing rules are evaluation is not changed, its should be only affecting the output of the parsers (in this sense a special PPActionParser).

...

2. For the same reason grammar transformation becomes impossible, one of the design goals of PetitParser.

sure, but having the feature doesn't mean you have to use it. And in my case it would simply some of the parsers I use. Actually in the end the Omit is nothing but a special PPActionParser right? (Just a curious question, how do you handle grammar transformation in this case?)

...

3. #omit makes a typical grammar (the smalltalk one) roughly 15% slower without even using the new feature.

yep thats an implementation issue, as you mentioned addressing this with a first-class object won't affect existing grammars at all.

...

4. There are various methods (#-, #match..., #copy..., ...) that need to be fixed when you add state to a parser.

5. There is no need for eager (#initialize) as well as lazy initialization (kills the possibility for a quick invocation).

right, thats indeed an ugly property of my hack (=> first class objects will do)

...

6. Please do not remove PPParser class>>new and do not change how initialization works, otherwise you break GemStone, VisualWorks, and GNU Smalltalk. The Seaside wiki has some nice documentation of how to correctly initialize objects (basically following the pattern of Objective-C).

ok, that was a bit hasty

...

I think, problems 1, 3 (!), 4, 5 and to some extent also 2 could be solved if the implementation was a bit more object-oriented. That is, if you added a PPOmitParser that would dispatch from #parseOn: to some other parsing method #parseOmittedOn: you wouldn't need all those #ifTrue:ifFalse:.

...

I pushed my solution to the petitparser repos. I didn't add full support for flatten.

Did you see #map: and #permutation:? Both allow you do the same in a less invasive way:

Of course in my example this is straight forward, but in a bit more complex parser with a couple of options or intermediate tokens you don't want it would be nicer to directly specify it on the parser nodes. And in such a sense only affect how flatten works...

...

parser := ($" asParser , $" asParser negate star flatten, $" asParser). parser map: [ :begin :inner :end | inner ]

or

parser := ($" asParser , $" asParser negate star flatten, $" asParser). parser permutation: #(2)

Additionally, did you see the experimental binding/capturing functionality in PetitBeta? This kind of provides something much more general and (I believe) much more powerful without the above drawbacks. Bind-Capture allows you to bind certain parse results anywhere down the tree to names and directly access them later on in a capture block. It also deals with unpacking and reshuffling collections automatically, check the tests for that.

Your example would look like:

parser := ($" asParser , ($" asParser negate star flatten bind: #inner) , $" asParser). parser capture: [ :inner | inner ]

Ah cool, this is indeed a bit nicer, didn't have at the new branch yet… will do ;)

In any case, the current code should be reverted. While you play with another implementation, could you move the current versions from the PetitParser repository?

I will remove it. I assume that the Helvetia image doesn't work anymore, because it always uses the latest version. Maybe changes (other than simple fixes) should be posted somewhere else to give some time for discussion before an eventual merge. Lukas -- Lukas Renggli www.lukas-renggli.ch

On 2011-08-11, at 15:26, Tudor Girba wrote:

Hi Camillo,

A first-class omit would be better especially given that you could package it as an extension.

In any case, the current code should be reverted. While you play with another implementation, could you move the current versions from the PetitParser repository?

sure sure remove it :)

Cheers, Doru

On 11 Aug 2011, at 15:14, Camillo Bruni wrote:

...

Thanks for the vast answers and pointing out critical parts of the implementation.

On 2011-08-11, at 08:14, Tudor Girba wrote:

...

Hi, Camillo, thanks for taking an interest and for letting us know that you committed.

Lukas, thanks for the answer. I would also like to add one comment: omit is maybe interesting for small and quick parsers in which you have the grammar and the parser together. However, for anything significant, if you want to split the grammar and the various parsers, it all of a sudden becomes less useful because you do not want to restrict the grammar.

Yeah and that's exactly the reason why I think this is useful. Since we want to use PetitParser also for small "dirty" projects where portability is a secondary problem. And I still disagree that it becomes less useful, it is just a way of filtering your output from parser in a very simple way ;). Furthermore having the omit doesn't mean that you will have to use it. t mean there is also the #flatten message which goes somewhat into the same direction IMO, since it is there for modifying directly in the parser the data. It could be omitted as well and put into the grammar.

...

I also took a quick look at the implementation. There are simply too many ifs that add complexity to an already non-trivial code.

Indeed the implementation is not very nice, and more of a preliminary hack to see if it works, I will definitely do a second pass with lukas' suggestion for a first-class object to denote the omit property.

...

All in all, I think we should retire this add-on.

Cheers, Doru

On 11 Aug 2011, at 07:31, Lukas Renggli wrote:

...

Hi Camillo,

...

I quickly hacked (with a lot of tests) support for omit in PetitParser. This should simplify some parsing efforts as an additional filtering step could be avoided.

parser := ($" asParser omit, $" asParser negate star flatten, $" asParser omit) parser parse: '"asdfsd"' "yields directly #('asdfsd') "

vs

parser := ($" asParser, $" asParser negate star flatten, $" asParser) parser parse: '"asdfsd"'

which yields #($" 'asdfsd' $")

I don't really see use of #omit. The element is still in a collection and you need to extract it from there. What is the benefit other than it is now at index 1 instead of 2?

Also the implementation has some problems:

1. #omit makes it really hard to compose grammars, because parsers suddenly get very asymmetric and start to affect each others behavior depending on how you compose them.

it shouldn't be about the behavior of the grammes, I might have not done everything correctly in my implementation, but the way the parsing rules are evaluation is not changed, its should be only affecting the output of the parsers (in this sense a special PPActionParser).

...

...

2. For the same reason grammar transformation becomes impossible, one of the design goals of PetitParser.

sure, but having the feature doesn't mean you have to use it. And in my case it would simply some of the parsers I use. Actually in the end the Omit is nothing but a special PPActionParser right? (Just a curious question, how do you handle grammar transformation in this case?)

...

...

3. #omit makes a typical grammar (the smalltalk one) roughly 15% slower without even using the new feature.

yep thats an implementation issue, as you mentioned addressing this with a first-class object won't affect existing grammars at all.

...

...

4. There are various methods (#-, #match..., #copy..., ...) that need to be fixed when you add state to a parser.

5. There is no need for eager (#initialize) as well as lazy initialization (kills the possibility for a quick invocation).

right, thats indeed an ugly property of my hack (=> first class objects will do)

...

...

6. Please do not remove PPParser class>>new and do not change how initialization works, otherwise you break GemStone, VisualWorks, and GNU Smalltalk. The Seaside wiki has some nice documentation of how to correctly initialize objects (basically following the pattern of Objective-C).

ok, that was a bit hasty

...

...

I think, problems 1, 3 (!), 4, 5 and to some extent also 2 could be solved if the implementation was a bit more object-oriented. That is, if you added a PPOmitParser that would dispatch from #parseOn: to some other parsing method #parseOmittedOn: you wouldn't need all those #ifTrue:ifFalse:.

...

I pushed my solution to the petitparser repos. I didn't add full support for flatten.

Did you see #map: and #permutation:? Both allow you do the same in a less invasive way:

Of course in my example this is straight forward, but in a bit more complex parser with a couple of options or intermediate tokens you don't want it would be nicer to directly specify it on the parser nodes. And in such a sense only affect how flatten works...

...

...

parser := ($" asParser , $" asParser negate star flatten, $" asParser). parser map: [ :begin :inner :end | inner ]

or

parser := ($" asParser , $" asParser negate star flatten, $" asParser). parser permutation: #(2)

Additionally, did you see the experimental binding/capturing functionality in PetitBeta? This kind of provides something much more general and (I believe) much more powerful without the above drawbacks. Bind-Capture allows you to bind certain parse results anywhere down the tree to names and directly access them later on in a capture block. It also deals with unpacking and reshuffling collections automatically, check the tests for that.

Your example would look like:

parser := ($" asParser , ($" asParser negate star flatten bind: #inner) , $" asParser). parser capture: [ :inner | inner ]

Ah cool, this is indeed a bit nicer, didn't have at the new branch yet… will do ;)

-- www.tudorgirba.com

"Some battles are better lost than fought."