Search code examples
c++boost-spirit-qi

Boost Spirit Qi crashes for memory violation

But I cannot figure out why...?

http://coliru.stacked-crooked.com/a/2912593bb421a35e

#include <boost/fusion/adapted/struct.hpp>
#include <boost/spirit/include/qi.hpp>

namespace bsq = boost::spirit::qi;

int main()
{        
    std::uint16_t major, minor, build, revision;

    auto versionParser =
        bsq::uint_
        >> -('.' >> bsq::uint_)
        >> -('.' >> bsq::uint_)
        >> -('.' >> bsq::uint_);

    std::string version = "3.5.1";

    auto start = version.begin();
    if (!bsq::parse(start, version.end(), versionParser, major, minor, build, revision))
    {
        std::cout << "Error!\n";
    }

    std::cout << major << "-" << minor << "-" << build << "-" << revision << std::endl;

    return 0;
}

The call to parse() causes a memory access violation.

I swear I had this working at one time but... maybe I was daydreaming. I've tried on Windows with Visual Studio 2017 and also on Coliru with clang. I cannot see the error.

Thank you.

Solution

  • The problem is the use of auto expressions to capture rules, which deduces the types from the parser expressions. That type is a proto-expression tree, which captures any relations by reference, but that means many of _the intermediates are gone after the end of the enclosing full-expresion (see C++: Life span of temporary arguments?).

    This is pretty well-known, as you can see here:

    Here's the simplest fix:

    auto versionParser = bsq::copy(
    bsq::uint_
    >> -('.' >> bsq::uint_)
    >> -('.' >> bsq::uint_)
    >> -('.' >> bsq::uint_));

    If you also fix the missing intialization of the local variables it works correctly:

    Live On Coliru

    #include <boost/fusion/adapted/struct.hpp>
#include <boost/spirit/include/qi.hpp>

namespace bsq = boost::spirit::qi;

int main()
{    
    std::cout << "BOOST_VERSION: " << BOOST_VERSION << std::endl;

    std::uint16_t major = 0, minor = 0, build = 0, revision = 0;

    auto versionParser = bsq::copy(
        bsq::uint_
        >> -('.' >> bsq::uint_)
        >> -('.' >> bsq::uint_)
        >> -('.' >> bsq::uint_));

    std::string version = "3.5.1";

    auto start = version.begin();
    if (!bsq::parse(start, version.end(), versionParser, major, minor, build, revision))
    {
        std::cout << "Error!\n";
    }

    std::cout << major << "-" << minor << "-" << build << "-" << revision << std::endl;
}

    Prints

    BOOST_VERSION: 106600
3-5-1-0

    Additional Notes

    1. To avoid the whole "unitialized attribute" situation, let's make it so the parser assigns to all elements, even if unspecified in the input text:

          >> ('.' >> bsq::uint_ | bsq::attr(0))
    >> ('.' >> bsq::uint_ | bsq::attr(0))
    >> ('.' >> bsq::uint_ | bsq::attr(0))

    2. To diagnose errors where there is trailing "garbage" (like with "3.4bogus"), you could add a check that the full input is parsed:

      auto versionParser = bsq::copy(
    bsq::uint_
    >> ('.' >> bsq::uint_ | bsq::attr(0))
    >> ('.' >> bsq::uint_ | bsq::attr(0))
    >> ('.' >> bsq::uint_ | bsq::attr(0))
    >> bsq::eoi);

    3. Because a version is semantically a tuple, why not represent it as such?

      using Version = std::tuple<uint16_t, uint16_t, uint16_t, uint16_t>;
Version parsed;

if (!bsq::parse(version.begin(), version.end(), versionParser, parsed))
    std::cout << "Error!\n";

      That way you can even say:

      using boost::fusion::operator<<;

auto obsolete = parsed < Version(3, 4, 0, 0);
std::cout << "Version " << parsed << " " << (obsolete? "too old" : "supported") << "\n";

    Combining those:

    Live On Coliru 

    #include <boost/fusion/adapted/std_tuple.hpp>
#include <boost/spirit/include/qi.hpp>

namespace bsq = boost::spirit::qi;

int main() {    
    auto versionParser = bsq::copy(
        bsq::uint_
        >> ('.' >> bsq::uint_ | bsq::attr(0))
        >> ('.' >> bsq::uint_ | bsq::attr(0))
        >> ('.' >> bsq::uint_ | bsq::attr(0))
        >> bsq::eoi);

    std::string version = "3.5.1";

    using Version = std::tuple<uint16_t, uint16_t, uint16_t, uint16_t>;
    Version parsed;

    if (!bsq::parse(version.begin(), version.end(), versionParser, parsed))
        std::cout << "Error!\n";

    using boost::fusion::operator<<;

    auto obsolete = parsed < Version(3, 4, 0, 0);
    std::cout << "Version " << parsed << " " << (obsolete? "too old" : "supported") << "\n";
}

    Prints

    Version (3 5 1 0) supported

    std::tuple sucks?

    I agree. So, equivalently write your own struct:

    Live On Coliru

    struct Version {
    uint16_t major, minor, revision, build;

    auto key() const { return std::tie(major, minor, revision, build); }
    bool operator<(Version const& b) const { return key() < b.key(); }
};

BOOST_FUSION_ADAPT_STRUCT(Version, major, minor, revision, build)

    Gettin' With The Times

    Note that Spirit X3 (Getting into boost spirit; Qi or X3?) doesn't have the auto-issues that you ran into:

    Live On Coliru

    #include <boost/fusion/adapted/struct.hpp>
#include <boost/spirit/home/x3.hpp>

#include <boost/fusion/include/io.hpp>
#include <iostream>

namespace bsx = boost::spirit::x3;

struct Version {
    uint16_t major, minor, revision, build;

    auto key() const { return std::tie(major, minor, revision, build); }
    bool operator<(Version const& b) const { return key() < b.key(); }
};

BOOST_FUSION_ADAPT_STRUCT(Version, major, minor, revision, build)

int main() {    
    auto versionParser = bsx::uint_
        >> ('.' >> bsx::uint_ | bsx::attr(0))
        >> ('.' >> bsx::uint_ | bsx::attr(0))
        >> ('.' >> bsx::uint_ | bsx::attr(0))
        >> bsx::eoi;

    std::string version = "3.5.1";

    Version parsed;

    if (!parse(version.begin(), version.end(), versionParser, parsed))
        std::cout << "Error!\n";

    using boost::fusion::operator<<;

    auto obsolete = parsed < Version{3, 4, 0, 0};
    std::cout << "Version " << parsed << " " << (obsolete? "too old" : "supported") << "\n";
}

    Also printing the same.