Problems with alternative in X3

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Problems with alternative in X3

Mikael Asplund

Hi!

 

I have an issue with alternatives that I don't understand why it's happening. I've simplified it even down to just x3::attr():s and I still get the problem.

 

Example here (code also supplied in attachment):

 

http://coliru.stacked-crooked.com/a/bdf4718cdeb2db4d

 

Why does the commented out version on line 57 work, and the active one on line 58-59 not work? It's an alternative of two exactly equal sequences, shouldn't that result in them joining to the attribute type as the single sequence?

 

Regards,

   Mikael

 


------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785351&iu=/4140
_______________________________________________
Spirit-general mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/spirit-general

alt_bug.cpp (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Problems with alternative in X3

Lee Clagett-2
On Wed, 23 Mar 2016 15:05:52 +0000
Mikael Asplund <[hidden email]> wrote:

> Hi!
>
> I have an issue with alternatives that I don't understand why it's
> happening. I've simplified it even down to just x3::attr():s and I
> still get the problem.
>
> Example here (code also supplied in attachment):
>
> http://coliru.stacked-crooked.com/a/bdf4718cdeb2db4d
>
> Why does the commented out version on line 57 work, and the active
> one on line 58-59 not work? It's an alternative of two exactly equal
> sequences, shouldn't that result in them joining to the attribute
> type as the single sequence?
>
> Regards,
>    Mikael
>

SpiritX3 does not collapse when alternative branches are identical.
Instead, X3 requires the attribute after the expect parser to be a
variant type [0]; only `variant<Foo>` is permitted as an attribute.
SpiritV2 would remove the variant type from the expected type if it
held only a single type element, allowing `Foo`, `optional<Foo>`, and
`variant<Foo>` in this situation. Joel, which is the preferred
behavior? I can work on a patch + tests if the SpiritV2 approach should
be taken.

Lee

[0] http://coliru.stacked-crooked.com/a/e92c2c33db69af38

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785351&iu=/4140
_______________________________________________
Spirit-general mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/spirit-general
Reply | Threaded
Open this post in threaded view
|

Re: Problems with alternative in X3

Joel de Guzman
On 24/03/2016 11:19 PM, Lee Clagett wrote:

> On Wed, 23 Mar 2016 15:05:52 +0000
> Mikael Asplund <[hidden email]> wrote:
>> Hi!
>>
>> I have an issue with alternatives that I don't understand why it's
>> happening. I've simplified it even down to just x3::attr():s and I
>> still get the problem.
>>
>> Example here (code also supplied in attachment):
>>
>> http://coliru.stacked-crooked.com/a/bdf4718cdeb2db4d
>>
>> Why does the commented out version on line 57 work, and the active
>> one on line 58-59 not work? It's an alternative of two exactly equal
>> sequences, shouldn't that result in them joining to the attribute
>> type as the single sequence?
>>
>> Regards,
>>     Mikael
>>
>
> SpiritX3 does not collapse when alternative branches are identical.
> Instead, X3 requires the attribute after the expect parser to be a
> variant type [0]; only `variant<Foo>` is permitted as an attribute.
> SpiritV2 would remove the variant type from the expected type if it
> held only a single type element, allowing `Foo`, `optional<Foo>`, and
> `variant<Foo>` in this situation. Joel, which is the preferred
> behavior? I can work on a patch + tests if the SpiritV2 approach should
> be taken.

Is this related to this ticket:

   https://svn.boost.org/trac/boost/ticket/12094

?

Cheers,
--
Joel de Guzman
http://www.ciere.com
http://boost-spirit.com
http://www.cycfi.com/


------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785351&iu=/4140
_______________________________________________
Spirit-general mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/spirit-general
Reply | Threaded
Open this post in threaded view
|

Re: Problems with alternative in X3

Lee Clagett-2
On Sat, 26 Mar 2016 08:26:06 +0800
Joel de Guzman <[hidden email]> wrote:

> On 24/03/2016 11:19 PM, Lee Clagett wrote:
> > On Wed, 23 Mar 2016 15:05:52 +0000
> > Mikael Asplund <[hidden email]> wrote:  
> >> Hi!
> >>
> >> I have an issue with alternatives that I don't understand why it's
> >> happening. I've simplified it even down to just x3::attr():s and I
> >> still get the problem.
> >>
> >> Example here (code also supplied in attachment):
> >>
> >> http://coliru.stacked-crooked.com/a/bdf4718cdeb2db4d
> >>
> >> Why does the commented out version on line 57 work, and the active
> >> one on line 58-59 not work? It's an alternative of two exactly
> >> equal sequences, shouldn't that result in them joining to the
> >> attribute type as the single sequence?
> >>
> >> Regards,
> >>     Mikael
> >>  
> >
> > SpiritX3 does not collapse when alternative branches are identical.
> > Instead, X3 requires the attribute after the expect parser to be a
> > variant type [0]; only `variant<Foo>` is permitted as an attribute.
> > SpiritV2 would remove the variant type from the expected type if it
> > held only a single type element, allowing `Foo`, `optional<Foo>`,
> > and `variant<Foo>` in this situation. Joel, which is the preferred
> > behavior? I can work on a patch + tests if the SpiritV2 approach
> > should be taken.  
>
> Is this related to this ticket:
>
>    https://svn.boost.org/trac/boost/ticket/12094
>

That ticket is closely related. I agree with your statement in the
ticket that the `parse_into_container_impl` specialization for the
`alternative` parser was probably a mistake. I found the commit [0] that
added this specialization, and found that it was added for cases
similar to this:

    std::string s;
    BOOST_TEST( (test_attr("ab",
        char_ >> char_ >> ((char_ % ',') | eps), s )) );

I think the problem was actually the difference in exposed attributes
between x3 and Qi:

    a: A, b: A --> (a | b): A          // Qi
    a: A, b: A --> (a | b): variant<A> // x3
 
So the exposed attribute above would be:

    sequence<char, char, container<char>>          // Qi
    sequence<char, char, variant<container<char>>> // x3

I tested a change to `attribute_of<alternative<...>>` to match the
behavior of Qi, and removed the `parse_into_impl` specializations for
`expect` and `alternative`. This forces the sequence parser to pass the
container to the alternative parser when both alternations collapse to
the same container type. All of the x3 tests pass, the code in the bug
ticket passes, but the code in the OP does not. The `static_assert`
still fails in detail/sequence.hpp because `sequence_size` counts the
size of the sequence in the alternative to be 1:

    a: A, b: B, c: C, d: B, e: C
        -> a >> ((b >> c) | (d >> e)) :
            sequence<A, B, C> but sequence_size<...>::value returns 2

I _think_ this case would work too if the static_assert were updated.
Also, it does not appear the alternative parser in x3 will consume
`optional` with a single variant type.

Lee

[0]https://github.com/boostorg/spirit/commit/f16822815a08bfeb70cf75c393d05343b7a7c9f5

------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785471&iu=/4140
_______________________________________________
Spirit-general mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/spirit-general
Reply | Threaded
Open this post in threaded view
|

Re: Problems with alternative in X3

Joel de Guzman
On 01/04/2016 1:02 PM, Lee Clagett wrote:

> On Sat, 26 Mar 2016 08:26:06 +0800
> Joel de Guzman <[hidden email]> wrote:
>> On 24/03/2016 11:19 PM, Lee Clagett wrote:
>>> On Wed, 23 Mar 2016 15:05:52 +0000
>>> Mikael Asplund <[hidden email]> wrote:
>>>> Hi!
>>>>
>>>> I have an issue with alternatives that I don't understand why it's
>>>> happening. I've simplified it even down to just x3::attr():s and I
>>>> still get the problem.
>>>>
>>>> Example here (code also supplied in attachment):
>>>>
>>>> http://coliru.stacked-crooked.com/a/bdf4718cdeb2db4d
>>>>
>>>> Why does the commented out version on line 57 work, and the active
>>>> one on line 58-59 not work? It's an alternative of two exactly
>>>> equal sequences, shouldn't that result in them joining to the
>>>> attribute type as the single sequence?
>>>>
>>>> Regards,
>>>>      Mikael
>>>>
>>>
>>> SpiritX3 does not collapse when alternative branches are identical.
>>> Instead, X3 requires the attribute after the expect parser to be a
>>> variant type [0]; only `variant<Foo>` is permitted as an attribute.
>>> SpiritV2 would remove the variant type from the expected type if it
>>> held only a single type element, allowing `Foo`, `optional<Foo>`,
>>> and `variant<Foo>` in this situation. Joel, which is the preferred
>>> behavior? I can work on a patch + tests if the SpiritV2 approach
>>> should be taken.
>>
>> Is this related to this ticket:
>>
>>     https://svn.boost.org/trac/boost/ticket/12094
>>
>
> That ticket is closely related. I agree with your statement in the
> ticket that the `parse_into_container_impl` specialization for the
> `alternative` parser was probably a mistake. I found the commit [0] that
> added this specialization, and found that it was added for cases
> similar to this:
>
>      std::string s;
>      BOOST_TEST( (test_attr("ab",
>          char_ >> char_ >> ((char_ % ',') | eps), s )) );
>
> I think the problem was actually the difference in exposed attributes
> between x3 and Qi:
>
>      a: A, b: A --> (a | b): A          // Qi
>      a: A, b: A --> (a | b): variant<A> // x3
>
> So the exposed attribute above would be:
>
>      sequence<char, char, container<char>>          // Qi
>      sequence<char, char, variant<container<char>>> // x3
>
> I tested a change to `attribute_of<alternative<...>>` to match the
> behavior of Qi, and removed the `parse_into_impl` specializations for
> `expect` and `alternative`. This forces the sequence parser to pass the
> container to the alternative parser when both alternations collapse to
> the same container type. All of the x3 tests pass, the code in the bug
> ticket passes, but the code in the OP does not. The `static_assert`
> still fails in detail/sequence.hpp because `sequence_size` counts the
> size of the sequence in the alternative to be 1:
>
>      a: A, b: B, c: C, d: B, e: C
>          -> a >> ((b >> c) | (d >> e)) :
>              sequence<A, B, C> but sequence_size<...>::value returns 2
>
> I _think_ this case would work too if the static_assert were updated.
> Also, it does not appear the alternative parser in x3 will consume
> `optional` with a single variant type.
>
> Lee
>
> [0]https://github.com/boostorg/spirit/commit/f16822815a08bfeb70cf75c393d05343b7a7c9f5

I think we should revert to the behavior of Qi, if possible. Can you make
a PR on your changes to the revert_to_qi_behavior branch I created just
now? Please add some relevant test cases too, if you can. That way,
we can work on this issue and hopefully merge back to develop when we
have a reasonable solution.

Thanks in advance!

Regards,
--
Joel de Guzman
http://www.ciere.com
http://boost-spirit.com
http://www.cycfi.com/


------------------------------------------------------------------------------
_______________________________________________
Spirit-general mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/spirit-general
Reply | Threaded
Open this post in threaded view
|

Re: Problems with alternative in X3

Lee Clagett-2
On Wed, 6 Apr 2016 00:11:59 +0800
Joel de Guzman <[hidden email]> wrote:

> On 01/04/2016 1:02 PM, Lee Clagett wrote:
> > On Sat, 26 Mar 2016 08:26:06 +0800
> > Joel de Guzman <[hidden email]> wrote:  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
> >
> > That ticket is closely related. I agree with your statement in the
> > ticket that the `parse_into_container_impl` specialization for the
> > `alternative` parser was probably a mistake. I found the commit [0]
> > that added this specialization, and found that it was added for
> > cases similar to this:
> >
> >      std::string s;
> >      BOOST_TEST( (test_attr("ab",
> >          char_ >> char_ >> ((char_ % ',') | eps), s )) );
> >
> > I think the problem was actually the difference in exposed
> > attributes between x3 and Qi:
> >
> >      a: A, b: A --> (a | b): A          // Qi
> >      a: A, b: A --> (a | b): variant<A> // x3
> >
> > So the exposed attribute above would be:
> >
> >      sequence<char, char, container<char>>          // Qi
> >      sequence<char, char, variant<container<char>>> // x3
> >
> > I tested a change to `attribute_of<alternative<...>>` to match the
> > behavior of Qi, and removed the `parse_into_impl` specializations
> > for `expect` and `alternative`. This forces the sequence parser to
> > pass the container to the alternative parser when both alternations
> > collapse to the same container type. All of the x3 tests pass, the
> > code in the bug ticket passes, but the code in the OP does not. The
> > `static_assert` still fails in detail/sequence.hpp because
> > `sequence_size` counts the size of the sequence in the alternative
> > to be 1:
> >
> >      a: A, b: B, c: C, d: B, e: C  
>  [...]  
> >              sequence<A, B, C> but sequence_size<...>::value
> > returns 2
> >
> > I _think_ this case would work too if the static_assert were
> > updated. Also, it does not appear the alternative parser in x3 will
> > consume `optional` with a single variant type.
> >
> > Lee
> >
> > [0]https://github.com/boostorg/spirit/commit/f16822815a08bfeb70cf75c393d05343b7a7c9f5 
>
> I think we should revert to the behavior of Qi, if possible. Can you
> make a PR on your changes to the revert_to_qi_behavior branch I
> created just now? Please add some relevant test cases too, if you
> can. That way, we can work on this issue and hopefully merge back to
> develop when we have a reasonable solution.
>
> Thanks in advance!
>
> Regards,

The X3 alternative parser now reports the expected attribute like Qi in
this branch. I have another open pull request which shows that
`boost::optional` works as expected in X3 without further code changes.
So I think X3 behavior now matches the documented behavior of Qi. The
code from the OP still does not compile, and I am unsure whether it
should be "fixed" since this would be a new technique for flattening
consumed fusion attributes.

Consider this trivial example which is a twist on the code from the OP:

    int_ >> (
        (':' >> int_ >> ':' >> int_) |
        (';' >> int_ >> ';' >> int_)
    ) >> '-' >> int_

The expected attribute is `tuple<int, tuple<int, int>, int>`. Should
`tuple<int, int, int, int>` also work? This flattened sequence is not
documented to work in Qi or X3. Qi compiles but generates incorrect
code, and X3 hits a helpful static_assert.

So perhaps this is worth a new thread - should the sequence operator
allow for the flattening of nested sequences? It _should_ be possible
to do this generically (i.e. no specializations for alternative), and
could be helpful in certain situations. Although this would mean that
any of the following consumed attributes would work:

    tuple<int, tuple<int int>, int, tuple<int, int>> // Expected
    tuple<int, tuple<int int>, int, int, int>
    tuple<int, int int, int, tuple<int, int>>
    tuple<int, int int, int, int, int>

Is that confusing?

Lee

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Spirit-general mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/spirit-general
Reply | Threaded
Open this post in threaded view
|

Re: Problems with alternative in X3

Joel de Guzman
On 13/04/2016 1:41 AM, Lee Clagett wrote:

> On Wed, 6 Apr 2016 00:11:59 +0800
> Joel de Guzman <[hidden email]> wrote:
>
>> On 01/04/2016 1:02 PM, Lee Clagett wrote:
>>> On Sat, 26 Mar 2016 08:26:06 +0800
>>> Joel de Guzman <[hidden email]> wrote:
>>   [...]
>>   [...]
>>   [...]
>>   [...]
>>   [...]
>>>
>>> That ticket is closely related. I agree with your statement in the
>>> ticket that the `parse_into_container_impl` specialization for the
>>> `alternative` parser was probably a mistake. I found the commit [0]
>>> that added this specialization, and found that it was added for
>>> cases similar to this:
>>>
>>>       std::string s;
>>>       BOOST_TEST( (test_attr("ab",
>>>           char_ >> char_ >> ((char_ % ',') | eps), s )) );
>>>
>>> I think the problem was actually the difference in exposed
>>> attributes between x3 and Qi:
>>>
>>>       a: A, b: A --> (a | b): A          // Qi
>>>       a: A, b: A --> (a | b): variant<A> // x3
>>>
>>> So the exposed attribute above would be:
>>>
>>>       sequence<char, char, container<char>>          // Qi
>>>       sequence<char, char, variant<container<char>>> // x3
>>>
>>> I tested a change to `attribute_of<alternative<...>>` to match the
>>> behavior of Qi, and removed the `parse_into_impl` specializations
>>> for `expect` and `alternative`. This forces the sequence parser to
>>> pass the container to the alternative parser when both alternations
>>> collapse to the same container type. All of the x3 tests pass, the
>>> code in the bug ticket passes, but the code in the OP does not. The
>>> `static_assert` still fails in detail/sequence.hpp because
>>> `sequence_size` counts the size of the sequence in the alternative
>>> to be 1:
>>>
>>>       a: A, b: B, c: C, d: B, e: C
>>   [...]
>>>               sequence<A, B, C> but sequence_size<...>::value
>>> returns 2
>>>
>>> I _think_ this case would work too if the static_assert were
>>> updated. Also, it does not appear the alternative parser in x3 will
>>> consume `optional` with a single variant type.
>>>
>>> Lee
>>>
>>> [0]https://github.com/boostorg/spirit/commit/f16822815a08bfeb70cf75c393d05343b7a7c9f5
>>
>> I think we should revert to the behavior of Qi, if possible. Can you
>> make a PR on your changes to the revert_to_qi_behavior branch I
>> created just now? Please add some relevant test cases too, if you
>> can. That way, we can work on this issue and hopefully merge back to
>> develop when we have a reasonable solution.
>>
>> Thanks in advance!
>>
>> Regards,
>
> The X3 alternative parser now reports the expected attribute like Qi in
> this branch. I have another open pull request which shows that
> `boost::optional` works as expected in X3 without further code changes.
> So I think X3 behavior now matches the documented behavior of Qi. The
> code from the OP still does not compile, and I am unsure whether it
> should be "fixed" since this would be a new technique for flattening
> consumed fusion attributes.
>
> Consider this trivial example which is a twist on the code from the OP:
>
>      int_ >> (
>          (':' >> int_ >> ':' >> int_) |
>          (';' >> int_ >> ';' >> int_)
>      ) >> '-' >> int_
>
> The expected attribute is `tuple<int, tuple<int, int>, int>`. Should
> `tuple<int, int, int, int>` also work? This flattened sequence is not
> documented to work in Qi or X3. Qi compiles but generates incorrect
> code, and X3 hits a helpful static_assert.
>
> So perhaps this is worth a new thread - should the sequence operator
> allow for the flattening of nested sequences? It _should_ be possible
> to do this generically (i.e. no specializations for alternative), and
> could be helpful in certain situations. Although this would mean that
> any of the following consumed attributes would work:
>
>      tuple<int, tuple<int int>, int, tuple<int, int>> // Expected
>      tuple<int, tuple<int int>, int, int, int>
>      tuple<int, int int, int, tuple<int, int>>
>      tuple<int, int int, int, int, int>
>
> Is that confusing?

I think I am satisfied with the current fixed behavior, getting as close
as possible to Qi. I do not think we should flatten sequences the way
it is presented in the code above, nor the OP's. There are ways to
get the desired behavior of the OP without having to wrestle with
the grammars. Simple is better and Qi/X3's behavior is already too
complicated and adding more complications can inadvertently cause
more fragility --one thing that X3 tries to address. At some point,
I even considered not inheriting the elaborate attribute heuristics
from Qi, hoping for frugal simplicity, but I decided not to because
doing so will make rules harder to write. Let us not add any more
complications.

Regards,
--
Joel de Guzman
http://www.ciere.com
http://boost-spirit.com
http://www.cycfi.com/


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Spirit-general mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/spirit-general