The state combinator blocks passing multiple state objects

[nlyan]

The state combinator, given state<S, G>, currently only passes a copy of state S to grammar Gs match() function. When multiple states, S1 and S2, are passed in to PEGTL, this results in G not seeing S2. I'm assuming this is an unindented oversight, as you're using a variadic param pack for state.

My simplistic workaround can be seen in my tree[0]. I've merely inverted the logic such that S1 is saved, and G is passed the original state pack in its entirety. If matching fails I call rollback(), rather than success(). This is obviously a breaking change, but I've found it incredibly useful to be able to handle granular state, so I'm interested in your thoughts.

Afaik this is a lot easier than doing the TMP dance to replace S1 in the state pack with a copy before calling match(), but maybe I'm missing your intent.

[0] nlyan@d16bc80

[ColinH]

Just to make sure I'm understanding the issue correctly, what you need is that if you start a parsing run with, say, two states s1 and s2, and you use state<> to change the state to s, that parsing continues with states s and s2, rather than only s?

[nlyan]

That's right. If you have two state classes paren_state and record_state, and protect a branch of grammar with state<paren_state, R> then R can no longer see record_state. The assumption seems to be that all parser state will be globbed in to a super object, which kind of breaks composability.

[d-frey]

It looks to me that state is meant for a completely different use-case. Even with the modifications you made, it is too complicated for what you need. How about a separate class like this:

// Copyright (c) 2016 Dr. Colin Hirsch and Daniel Frey
// Please see LICENSE for license or visit https://github.com/ColinH/PEGTL/

#ifndef PEGTL_INTERNAL_TRANSACTION_HH
#define PEGTL_INTERNAL_TRANSACTION_HH

#include <tuple>
#include <type_traits>

#include "seq.hh"
#include "rule_match_three.hh"
#include "skip_control.hh"

#include "../analysis/generic.hh"

namespace pegtl
{
   namespace internal
   {
      template< typename ... Rules >
      struct transaction
      {
         using analyze_t = analysis::generic< analysis::rule_type::SEQ, Rules ... >;

         template< apply_mode A, template< typename ... > class Action, template< typename ... > class Control, typename Input, typename ... States >
         static bool match( Input & in, States && ... st )
         {
            const std::tuple< typename std::decay< States >::type ... > s( st ... );
            if ( rule_match_three< seq< Rules ... >, A, Action, Control >::match( in, st ... ) ) {
               std::tie( st ... ) = s;
               return false;
            }
            return true;
         }
      };

      template< typename ... Rules >
      struct skip_control< transaction< Rules ... > > : std::true_type {};

   } // internal

} // pegtl

#endif

Note that I haven't tested the above, it might need some small weaks.

@nlyan: Would you think that the above can work in your case? Or is your use-case more difficult?

[nlyan]

d-frey, the problem with that approach is that it saves and restores (i.e. copies) all state. Here's the approach I'm currently taking.

struct paren_state {
    unsigned long depth = 0;
    paren_state() noexcept = default;

    template <typename... States>
    paren_state (pegtl::input const&, States&&... states) noexcept
        : paren_state(select<paren_state>(std::forward<States>(states)...)) {
    }

    template <typename... States>
    void
    rollback (pegtl::input const&, States&&... states) const noexcept {
        auto& ps = select<paren_state>(std::forward<States>(states)...);
        ps = *this;
    }
};

select<T>(...) is a simply a helper which selects the first T, by reference, from the arguments passed to it.

Inside state<paren_state,...>, all the active states are first passed to the paren_state constructor, which, in this case, simply ignores everything except the active paren_state and delegates to the copy-constructor. This establishes the backup of paren_state. Now the original state set ("st..." in state<>::match()) is passed to the grammar. With my rollback() patch, if it fails, paren_state::rollback() is then called on the backup, which again filters out everything except the active (but now possibly modified) paren_state, and delegates to the assignment operator to restore the original state. For extra efficiency, std::move can even be used here.

A variant of this code was working with master until I realised state<>::match was only passing the copy to the grammar.

You might be right that moving the rollback code in to a new state combinator is better than handling it like paren_state above, but nonetheless, as it stands, something like seq<Rule1, Rule2> can compile, whereas state<SomeState, Rule1, Rule2> can easily not unless you're willing to change Rule1 and Rule2 invasively.

[d-frey]

Concerning your last paragraph "nonetheless...": The use-case state was designed for is very different from what you are trying to do, it is meant to have a different sub-grammar G that only know about the one new state S passed to G, not about the enclosing states. Only S itself should know about the enclosing states and how to connect the two worlds.

That said, let's concentrate on how to create a generally useful and generic helper that works for your use-case. I will think about it some more and discuss it with Colin.

[ColinH]

We have occasionally looked into adding more general or powerful variants of state<>, however different use cases often had slightly different requirements. In order to prevent ending up with a confusing set of very similar classes, we decided to gather more experience and see which use cases are prevalent... As Daniel said, we'll see what we can come up with, until then you'll have to use your own solution, and thank your for sharing it with us, this kind of feedback is always very valuable input for our further development!

[nlyan]

Thanks guys. Once I'm done with the parser I'm writing, I'll also put some more thought in to this.

[ColinH]

So, we had a good chat about this issue, but at the moment we can't see a solution that is noticeably more general than the included state<> and change_state<> without being either very complicated or having so many requirements regarding the user's code that it isn't really worth it.

In other words, with our current experience you are already doing the right thing: Take the supplied code as example, and implement your state-changing class that exactly fits your use case...

But please do continue to show us exactly at which points you need to go beyond the supplied solutions, and how exactly. That might help us when we revisit this subject further down the road. For now we will close this issue.