Merge lp:~thomir-deactivatedaccount/xpathselect/trunk-extend-type-name-options into lp:xpathselect

[Thomi Richards]

Extend xpathselect grammar to allow multiple node names per query.

This branch changes the XPathSelect grammar such that you can now say "get me all the nodes named X, Y, or Z at this location". This is required to work around a problem with versioned Qml objects, and is also useful in other scenarios.

This involved extending the boost::spirit grammar (in parser.h), which in turn meant re-working the parser return types to be different structs, and using boost::variant.

As you'd expect, tests have been added / updated.

[Thomi Richards (thomir-deactivatedaccount)]

Note: Please do not merge this. While this MP is considered complete, I need to write the autopilot-side of things, and I'd like to land the two at the same time to avoid back-and-forth merge proposals between the two projects.

Reviews are very welcome though :)

[Brendan Donegan (brendan-donegan)]

That's a whole lotta code to parse! A couple of probably useless comments for you though.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:64
http://jenkins.qa.ubuntu.com/job/xpathselect-ci/8/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/xpathselect-utopic-amd64-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/xpathselect-utopic-armhf-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/xpathselect-utopic-i386-ci/1

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/xpathselect-ci/8/rebuild

review: Approve (continuous-integration)

[Christopher Lee (veebers)]

Just a quick look at this point, couple of inline comments re; TODOs. Deeper review to come.

[lp:~thomir-deactivatedaccount/xpathselect/trunk-extend-type-name-options]

65. By Thomi Richards on 2014-07-23

Code cleanups.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:65
http://jenkins.qa.ubuntu.com/job/xpathselect-ci/10/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/xpathselect-utopic-amd64-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/xpathselect-utopic-armhf-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/xpathselect-utopic-i386-ci/3

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/xpathselect-ci/10/rebuild

review: Approve (continuous-integration)

[Martin Pitt (pitti)]

I didn't see anything obviously wrong with this, but then again my C++ skills are largely nonexisting, and most of the syntax is totally unfamiliar to me :/ Just two small and unimportant comments. But the test case coverage of this is quite good, and I suppose you'll test it together with the AP testsuite.

[lp:~thomir-deactivatedaccount/xpathselect/trunk-extend-type-name-options]

66. By Thomi Richards on 2014-07-28

Fix test as per MP comment.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:66
http://jenkins.qa.ubuntu.com/job/xpathselect-ci/11/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/xpathselect-utopic-amd64-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/xpathselect-utopic-armhf-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/xpathselect-utopic-i386-ci/4

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/xpathselect-ci/11/rebuild

review: Approve (continuous-integration)

Unmerged revisions

66. By Thomi Richards on 2014-07-28

Fix test as per MP comment.

65. By Thomi Richards on 2014-07-23

Code cleanups.

64. By Thomi Richards on 2014-07-23

Remove use of boost::optional that wasn't needed.

63. By Thomi Richards on 2014-07-23

whitespace-only change.

62. By Thomi Richards on 2014-07-23

Addded test, cleaned up the code a little bit.

61. By Thomi Richards on 2014-07-23

Remove some commented out code.

60. By Thomi Richards on 2014-07-23

Add missing file - whoops!

59. By Thomi Richards on 2014-07-23

Use a template function to remove some code.

58. By Thomi Richards on 2014-07-23

Hook up new xpathselect grammar, added test to those already present.

57. By Thomi Richards on 2014-07-23

Builds, and tests all pass again.

=== modified file 'lib/CMakeLists.txt'
--- lib/CMakeLists.txt	2013-01-22 02:39:32 +0000
+++ lib/CMakeLists.txt	2014-07-28 19:46:11 +0000
@@ -1,11 +1,11 @@
 FIND_PACKAGE( Boost 1.40 REQUIRED )
 INCLUDE_DIRECTORIES( ${Boost_INCLUDE_DIR} )
 
-set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC -Wall -Wl,--no-undefined")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC -Wall -Wl,--no-undefined -ftemplate-backtrace-limit=0")
 if(CMAKE_BUILD_TYPE STREQUAL "Debug")
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DDEBUG" )
 endif(CMAKE_BUILD_TYPE STREQUAL "Debug")
-set(SOURCES node.cpp xpathselect.h xpathselect.cpp parser.cpp parser.h)
+set(SOURCES node.cpp xpathselect.h xpathselect.cpp xpathquerypart.cpp parser.cpp parser.h)
 set(HEADERS node.h xpathselect.h)
 
 if(CMAKE_COMPILER_IS_GNUCXX)

=== modified file 'lib/parser.cpp'
--- lib/parser.cpp	2013-01-21 02:27:45 +0000
+++ lib/parser.cpp	2014-07-28 19:46:11 +0000
@@ -1,5 +1,5 @@
 /*
-* Copyright (C) 2013 Canonical Ltd
+* Copyright (C) 2013-2014 Canonical Ltd
 *
 * This program is free software: you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 3 as
@@ -16,3 +16,36 @@
 */
 
 #include "parser.h"
+
+xpathselect::QueryList xpathselect::parser::parse_query(std::string const& query)
+{
+    bool s_;
+    return parse_query(query, s_);
+}
+
+xpathselect::QueryList xpathselect::parser::parse_query(std::string const& query, bool& success)
+{
+    xpathselect::parser::xpath_grammar<std::string::const_iterator> grammar;
+    QueryList query_parts;
+    NodeSequence results;
+
+    auto begin = query.cbegin();
+    auto end = query.cend();
+    if (boost::spirit::qi::parse(begin, end, grammar, results) && (begin == end))
+    {
+        success = true;
+        for (auto n: results)
+        {
+            if (auto p1 = boost::fusion::at_c<0>(n))
+            {
+                query_parts.push_back(xpathselect::AnyNode(*p1));
+            }
+            query_parts.push_back(boost::fusion::at_c<1>(n));
+        }
+    }
+    else
+    {
+        success = false;
+    }
+    return query_parts;
+}

=== modified file 'lib/parser.h'
--- lib/parser.h	2014-02-19 19:18:58 +0000
+++ lib/parser.h	2014-07-28 19:46:11 +0000
@@ -1,5 +1,5 @@
 /*
-* Copyright (C) 2013 Canonical Ltd
+* Copyright (C) 2013-2014 Canonical Ltd
 *
 * This program is free software: you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 3 as
@@ -22,26 +22,27 @@
 
 #include <boost/config/warning_disable.hpp>
 #include <boost/fusion/include/adapt_struct.hpp>
-#include <boost/spirit/include/phoenix_object.hpp>
-#include <boost/spirit/include/phoenix_operator.hpp>
-#include <boost/spirit/include/qi.hpp>
-#include <boost/spirit/include/qi_bool.hpp>
-#include <boost/spirit/include/qi_int.hpp>
+#include <boost/spirit/include/phoenix.hpp>
+#include <boost/spirit/include/qi.hpp> 
 
 #include "xpathquerypart.h"
 
-// this allows spirit to lazily construct these two structs...
-BOOST_FUSION_ADAPT_STRUCT(
-    xpathselect::XPathQueryPart,
-    (std::string, node_name_)
-    (xpathselect::ParamList, parameter)
-    );
+BOOST_FUSION_ADAPT_STRUCT(
+    xpathselect::XPathWildcardNode,
+    (xpathselect::ParamList, parameters)
+);
+
+BOOST_FUSION_ADAPT_STRUCT(
+    xpathselect::XPathSpecifiedNode,
+    (xpathselect::NodeNameList, names)
+    (xpathselect::ParamList, parameters)
+);
 
 BOOST_FUSION_ADAPT_STRUCT(
     xpathselect::XPathQueryParam,
     (std::string, param_name)
     (xpathselect::XPathQueryParam::ParamValueType, param_value)
-    );
+);
 
 namespace xpathselect
 {
@@ -85,7 +86,7 @@
     // on - it must adhere to std::forward_iterator. The second template parameter is the type
     // that this grammar will produce (in this case: a list of XPathQueryPart objects).
     template <typename Iterator>
-    struct xpath_grammar : qi::grammar<Iterator, QueryList()>
+    struct xpath_grammar : qi::grammar<Iterator, NodeSequence()>
     {
         xpath_grammar() : xpath_grammar::base_type(node_sequence) // node_sequence is the start rule.
         {
@@ -135,80 +136,75 @@
             // it must start and end with a non-space character, but you can have
             // spaces in the middle.
             spec_node_name = +qi::char_("a-zA-Z0-9_\\-") >> *(+qi::char_(" :") >> +qi::char_("a-zA-Z0-9_\\-"));
+            spec_node_name_list = spec_node_name % ',';
             // a wildcard node name is simply a '*'
-            wildcard_node_name = qi::char_("*");
+            wildcard_node_name = "*";
 
 
             // a spec_node consists of a specified node name, followed by an *optional* parameter list.
-            spec_node %= spec_node_name >> -(param_list);
+            spec_node %= spec_node_name_list >> -(param_list);
             // a wildcard node is a '*' without parameters:
             wildcard_node %= wildcard_node_name >> !param_list;
             // wildcard nodes can also have parameters:
             wildcard_node_with_params %= wildcard_node_name >> param_list;
             // A parent node is '..' as long as it's followed by a normal separator or end of input:
-            parent_node = qi::lit("..")[qi::_val = XPathQueryPart("..")];
+            parent_node = qi::lit("..") >> qi::attr(XPathParentNode());
 
             // node is simply any kind of code defined thus far:
-            node = spec_node | wildcard_node_with_params | wildcard_node | parent_node;
+            any_node = spec_node | wildcard_node_with_params | wildcard_node | parent_node;
 
             // a search node is '//' as long as it's followed by a spec node or a wildcard node with parameters.
             // we don't allow '//*' since it would match everything in the tree, and cause HUGE amounts of
             // data to be transmitted.
-            search_node = "//" >> &(spec_node | wildcard_node_with_params)[qi::_val = XPathQueryPart()];
-
+            search_node = qi::lit("//") >> &(spec_node | wildcard_node_with_params) >> qi::attr(XPathSearchNode());
 
             // a normal separator is a '/' as long as it's followed by something other than another '/'
             normal_sep = '/' >> !qi::lit('/');
-            separator = normal_sep | search_node;  // nodes can be separated by normal_sep or search_node.
+            separator = search_node | normal_sep;  // nodes can be separated by normal_sep or search_node.
             // this is the money shot: a node sequence is one or more of a separator, followed by an
             // optional node.
-            node_sequence %= +(separator >> -node);
+            sep_node_pair = separator >> any_node;
+            node_sequence = +sep_node_pair;
 
             // DEBUGGING SUPPORT:
             // define DEBUG in order to have boost::spirit spit out useful debug information:
 #ifdef DEBUG
             // this gives english names to all the grammar rules:
             spec_node_name.name("spec_node_name");
+            spec_node_name_list.name("spec_node_name_list");
             wildcard_node_name.name("wildcard_node_name");
             search_node.name("search_node");
-            normal_sep.name("normal_separator");
+            parent_node.name("parent_node");
+            normal_sep.name("normal_sep");
             separator.name("separator");
             param_name.name("param_name");
             param_value.name("param_value");
             param.name("param");
+            param_list.name("param_list");
             spec_node.name("spec_node");
             wildcard_node.name("wildcard_node");
-            wildcard_node.name("wildcard_node_with_params");
-            node.name("node");
+            wildcard_node_with_params.name("wildcard_node_with_params");
+            any_node.name("any_node");
+            sep_node_pair.name("sep_node_pair");
             node_sequence.name("node_sequence");
-            param_list.name("param_list");
 
-            // set up error logging:
-            qi::on_error<qi::fail>(
-                node_sequence,
-                std::cout
-                    << phoenix::val("Error! Expecting ")
-                    << qi::_4                               // what failed?
-                    << phoenix::val(" here: \"")
-                    << phoenix::construct<std::string>(qi::_3, qi::_2)   // iterators to error-pos, end
-                    << phoenix::val("\"")
-                    << std::endl
-            );
-            // specify which rules we want debug info about (all of them):
             qi::debug(spec_node_name);
+            qi::debug(spec_node_name_list);
             qi::debug(wildcard_node_name);
             qi::debug(search_node);
+            qi::debug(parent_node);
             qi::debug(normal_sep);
             qi::debug(separator);
             qi::debug(param_name);
             qi::debug(param_value);
             qi::debug(param);
+            qi::debug(param_list);
             qi::debug(spec_node);
             qi::debug(wildcard_node);
             qi::debug(wildcard_node_with_params);
-            qi::debug(node);
+            qi::debug(any_node);
+            qi::debug(sep_node_pair);
             qi::debug(node_sequence);
-            qi::debug(param_list);
 #endif
         }
         // declare all the rules. The second template parameter is the type they produce.
@@ -222,30 +218,39 @@
         // symbol table for chracter scape codes.
         qi::symbols<char const, char const> unesc_char;
 
-        // parse integers, first signed then unsigned:
+        // parse integers:
         qi::rule<Iterator, int32_t()> int_type;
 
         // more complicated language rules:
         qi::rule<Iterator, std::string()> spec_node_name;
-        qi::rule<Iterator, std::string()> wildcard_node_name;
-        qi::rule<Iterator, XPathQueryPart()> search_node;
-        qi::rule<Iterator, XPathQueryPart()> parent_node;
+        qi::rule<Iterator, xpathselect::NodeNameList()> spec_node_name_list;
+        qi::rule<Iterator> wildcard_node_name;
+        qi::rule<Iterator, XPathSearchNode()> search_node;
+        qi::rule<Iterator, XPathParentNode()> parent_node;
         qi::rule<Iterator> normal_sep;
-        qi::rule<Iterator, xpathselect::QueryList()> separator;
+        qi::rule<Iterator, xpathselect::MaybeSearchNode()> separator;
 
         qi::rule<Iterator, std::string()> param_name;
         qi::rule<Iterator, xpathselect::XPathQueryParam::ParamValueType()> param_value;
         qi::rule<Iterator, XPathQueryParam()> param;
         qi::rule<Iterator, xpathselect::ParamList()> param_list;
 
-        qi::rule<Iterator, XPathQueryPart()> spec_node;
-        qi::rule<Iterator, XPathQueryPart()> wildcard_node;
-        qi::rule<Iterator, XPathQueryPart()> wildcard_node_with_params;
-        qi::rule<Iterator, XPathQueryPart()> node;
+        qi::rule<Iterator, XPathSpecifiedNode()> spec_node;
+        qi::rule<Iterator, XPathWildcardNode()> wildcard_node;
+        qi::rule<Iterator, XPathWildcardNode()> wildcard_node_with_params;
+        qi::rule<Iterator, AnyNode()> any_node;
+        qi::rule<Iterator, SepNodePair()> sep_node_pair;
 
-        qi::rule<Iterator, xpathselect::QueryList()> node_sequence;
+        qi::rule<Iterator, NodeSequence()> node_sequence;
     };
 
+    /// Take some input, parse it, and return a flattened data structure,
+    /// which is what most people actually want (means they don't need to
+    /// deal with boost::optional components)
+    QueryList parse_query(std::string const& query);
+    /// Similar to the above, but allows the caller to determine if the grammar
+    /// parsed the whole input or not.
+    QueryList parse_query(std::string const& query, bool& success);
 }
 }
 

=== added file 'lib/xpathquerypart.cpp'
--- lib/xpathquerypart.cpp	1970-01-01 00:00:00 +0000
+++ lib/xpathquerypart.cpp	2014-07-28 19:46:11 +0000
@@ -0,0 +1,129 @@
+/*
+* Copyright (C) 2013 Canonical Ltd
+*
+* This program is free software: you can redistribute it and/or modify
+* it under the terms of the GNU General Public License version 3 as
+* published by the Free Software Foundation.
+*
+* This program is distributed in the hope that it will be useful,
+* but WITHOUT ANY WARRANTY; without even the implied warranty of
+* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+* GNU General Public License for more details.
+*
+* You should have received a copy of the GNU General Public License
+* along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*
+*/
+
+#include <string>
+#include <vector>
+#include <memory>
+#include <algorithm>
+#include <iostream>
+
+#include <boost/optional/optional.hpp>
+#include <boost/variant/variant.hpp>
+#include <boost/variant/get.hpp>
+
+#include "xpathquerypart.h"
+#include "node.h"
+
+namespace xpathselect
+{
+    namespace 
+    {
+        bool nodeMatchesParameters(Node::Ptr const& node, ParamList const& parameters)
+        {
+            bool matches = true;
+            if (!parameters.empty())
+            {
+                for (auto param : parameters)
+                {
+                    switch(param.param_value.which())
+                    {
+                        case 0:
+                        {
+                            matches &= node->MatchStringProperty(param.param_name, boost::get<std::string>(param.param_value));
+                        }
+                        break;
+                        case 1:
+                        {
+                            matches &= node->MatchBooleanProperty(param.param_name, boost::get<bool>(param.param_value));
+                        }
+                        break;
+                        case 2:
+                        {
+                            matches &= node->MatchIntegerProperty(param.param_name, boost::get<int>(param.param_value));
+                        }
+                        break;
+                    }
+                }
+            }
+            return matches;
+        }
+    }
+    
+#ifdef DEBUG
+    std::string XPathSearchNode::Dump() const
+    {
+        return "SearchNode";
+    }
+
+    std::string XPathParentNode::Dump() const
+    {
+        return "ParentNode";
+    }
+
+    std::string XPathWildcardNode::Dump() const
+    {
+        return "WildcardNode";
+    }
+
+    std::string XPathSpecifiedNode::Dump() const
+    {
+        std::string s("SpecifiedNode(");
+        for (auto n: names)
+            s += n + " ";
+        s += ")";
+        return s;
+    }
+#endif
+
+    bool XPathWildcardNode::Matches(Node::Ptr const& node) const
+    {
+        return nodeMatchesParameters(node, parameters);
+    }
+
+    bool XPathSpecifiedNode::Matches(Node::Ptr const& node) const
+    {
+        bool matches = std::find(names.cbegin(), names.cend(), node->GetName()) != names.cend();
+        return matches && nodeMatchesParameters(node, parameters);
+    }
+
+#ifdef DEBUG
+    std::ostream &operator<<(std::ostream &stream, XPathWildcardNode const& ob)
+    {
+      stream << ob.Dump() << ' ';
+      return stream;
+    }
+
+    std::ostream &operator<<(std::ostream &stream, XPathSpecifiedNode const& ob)
+    {
+      stream << ob.Dump() << ' ';
+      return stream;
+    }
+
+    std::ostream &operator<<(std::ostream &stream, XPathSearchNode const& ob)
+    {
+      stream << ob.Dump() << ' ';
+      return stream;
+    }
+
+    std::ostream &operator<<(std::ostream &stream, XPathParentNode const& ob)
+    {
+      stream << ob.Dump() << ' ';
+      return stream;
+    }
+#endif
+    
+}

=== modified file 'lib/xpathquerypart.h'
--- lib/xpathquerypart.h	2013-09-03 03:28:14 +0000
+++ lib/xpathquerypart.h	2014-07-28 19:46:11 +0000
@@ -1,5 +1,5 @@
 /*
-* Copyright (C) 2013 Canonical Ltd
+* Copyright (C) 2013-2014 Canonical Ltd
 *
 * This program is free software: you can redistribute it and/or modify
 * it under the terms of the GNU General Public License version 3 as
@@ -20,12 +20,10 @@
 
 #include <string>
 #include <vector>
-#include <memory>
-#include <iostream>
 
 #include <boost/optional/optional.hpp>
 #include <boost/variant/variant.hpp>
-#include <boost/variant/get.hpp>
+#include <boost/fusion/include/vector.hpp>
 
 #include "node.h"
 
@@ -40,76 +38,62 @@
         std::string param_name;
         ParamValueType param_value;
     };
-
+    
     typedef std::vector<XPathQueryParam> ParamList;
-
-    // Stores a part of an XPath query.
-    struct XPathQueryPart
-    {
-    public:
-        XPathQueryPart() {}
-        XPathQueryPart(std::string node_name)
-        : node_name_(node_name)
-        {}
-
-        enum class QueryPartType {Normal, Search, Parent};
-
-        bool Matches(Node::Ptr const& node) const
-        {
-            bool matches = (node_name_ == "*" || node->GetName() == node_name_);
-            if (!parameter.empty())
-            {
-                for (auto param : parameter)
-                {
-                    switch(param.param_value.which())
-                    {
-                        case 0:
-                        {
-                            matches &= node->MatchStringProperty(param.param_name, boost::get<std::string>(param.param_value));
-                        }
-                        break;
-                        case 1:
-                        {
-                            matches &= node->MatchBooleanProperty(param.param_name, boost::get<bool>(param.param_value));
-                        }
-                        break;
-                        case 2:
-                        {
-                            matches &= node->MatchIntegerProperty(param.param_name, boost::get<int>(param.param_value));
-                        }
-                        break;
-                    }
-                }
-            }
-
-            return matches;
-        }
-
-        QueryPartType Type() const
-        {
-            if (node_name_ == "")
-                return QueryPartType::Search;
-            else if (node_name_ == "..")
-                return QueryPartType::Parent;
-            else
-                return QueryPartType::Normal;
-        }
-
-        void Dump() const
-        {
-            if (Type() == QueryPartType::Search)
-                std::cout << "<search> ";
-            else
-                std::cout << "[" << node_name_ << "] ";
-        }
-
-        std::string node_name_;
-        ParamList parameter;
-    };
-
-
-
-    typedef std::vector<XPathQueryPart> QueryList;
+    typedef std::vector<std::string> NodeNameList;
+   
+    struct XPathSearchNode
+    {
+#ifdef DEBUG
+        std::string Dump() const;
+#endif
+    };
+
+    struct XPathParentNode
+    {
+#ifdef DEBUG
+        std::string Dump() const;
+#endif
+    };
+
+    struct XPathWildcardNode
+    {
+        bool Matches(Node::Ptr const& node) const;
+#ifdef DEBUG
+        std::string Dump() const;
+#endif
+        ParamList parameters;
+    };
+
+    struct XPathSpecifiedNode
+    {
+        bool Matches(Node::Ptr const& node) const;
+#ifdef DEBUG
+        std::string Dump() const;
+#endif
+        NodeNameList names;
+        ParamList parameters;
+    };
+
+#ifdef DEBUG
+    std::ostream &operator<<(std::ostream &stream, XPathWildcardNode const& ob);
+    std::ostream &operator<<(std::ostream &stream, XPathSpecifiedNode const& ob);
+    std::ostream &operator<<(std::ostream &stream, XPathSearchNode const& ob);
+    std::ostream &operator<<(std::ostream &stream, XPathParentNode const& ob);    
+#endif
+
+    typedef boost::variant<
+        XPathSearchNode, 
+        XPathParentNode, 
+        XPathWildcardNode, 
+        XPathSpecifiedNode
+    > AnyNode;
+    typedef boost::optional<XPathSearchNode> MaybeSearchNode;
+
+    typedef boost::fusion::vector<MaybeSearchNode, AnyNode> SepNodePair;
+    typedef std::vector<SepNodePair> NodeSequence;
+
+    typedef std::vector<AnyNode> QueryList;
 
 }
 

=== modified file 'lib/xpathselect.cpp'
--- lib/xpathselect.cpp	2013-09-03 03:28:14 +0000
+++ lib/xpathselect.cpp	2014-07-28 19:46:11 +0000
@@ -25,35 +25,76 @@
 
 namespace xpathselect
 {
+#ifdef DEBUG
+    namespace debug
+    {
+        class dump_visitor: public boost::static_visitor<std::string>
+        {
+        public:
+            
+            template <typename T>
+            std::string operator()(const T& t) const
+            {
+                return t.Dump();
+            }
+        };        
+
+        void dump_query_list(QueryList const& ql)
+        {
+            std::cout << "Query parts are: ";
+            for (auto n : ql)
+                std::cout << boost::apply_visitor(dump_visitor(), n) << " ";
+            std::cout << std::endl;
+        }
+    }
+#endif
     // anonymous namespace for internal-only utility class:
     namespace
     {
         QueryList GetQueryPartsFromQuery(std::string const& query)
         {
-            xpathselect::parser::xpath_grammar<std::string::const_iterator> grammar;
-            QueryList query_parts;
-
-            auto begin = query.cbegin();
-            auto end = query.cend();
-            if (boost::spirit::qi::parse(begin, end, grammar, query_parts) && (begin == end))
-            {
-#ifdef DEBUG
-                std::cout << "Query parts are: ";
-                for (auto n : query_parts)
-                    n.Dump();
-                std::cout << std::endl;
-#endif
-                return query_parts;
-            }
-#ifdef DEBUG
-            std::cout << "Query failed." << std::endl;
-#endif
-            return QueryList();
+            QueryList query_parts = xpathselect::parser::parse_query(query);
+#ifdef DEBUG
+            ::xpathselect::debug::dump_query_list(*query_parts);
+#endif
+            return query_parts;
+        }
+
+        class matches_visitor: public boost::static_visitor<bool>
+        {
+        public:
+            matches_visitor(Node::Ptr const& n)
+            : node_(n) {}
+
+            bool operator()(XPathSpecifiedNode const& n) const
+            {
+                return n.Matches(node_);
+            }
+
+            bool operator()(XPathWildcardNode const& n) const
+            {
+                return n.Matches(node_);
+            }
+
+            // everything else:
+            template <typename U>
+            bool operator()(U n) const
+            {
+                return false;
+            }
+
+            Node::Ptr const& node_;
+        };
+
+        // return true if 'p' is one of the matchable types, and p matches 'node'
+        bool maybe_matches(Node::Ptr const& node, AnyNode const& p)
+        {
+            return boost::apply_visitor( matches_visitor(node), p );
         }
 
         // Starting at each node listed in 'start_points', search the tree for nodes that match
         // 'next_match'. next_match *must* be a normal query part object, not a search token.
-        NodeList SearchTreeForNode(NodeList const& start_points, XPathQueryPart const& next_match)
+        NodeList SearchTreeForNode(NodeList const& start_points, AnyNode const& next_match)
         {
             NodeList matches;
             for (auto root: start_points)
@@ -65,7 +106,8 @@
                 {
                     Node::Ptr node = queue.front();
                     queue.pop();
-                    if (next_match.Matches(node))
+
+                    if (maybe_matches(node, next_match))
                     {
                         // found one. We keep going deeper, as there may be another node beneath this one
                         // with the same node name.
@@ -98,20 +140,16 @@
         while (query_part != query_parts.cend())
         {
             // If the current query piece is a recursive search token ('//')...
-            if (query_part->Type() == XPathQueryPart::QueryPartType::Search)
+            if (query_part->type() == typeid(XPathSearchNode))
             {
                 // advance to look at the next piece.
                 ++query_part;
-                // do some sanity checking...
-                if (query_part->Type() == XPathQueryPart::QueryPartType::Search)
-                    // invalid query - cannot specify multiple search sequences in a row.
-                    return NodeVector();
+
                 // then find all the nodes that match the new query part, and store them as
-                // the new start nodes. We pass in 'start_nodes' rather than 'root' since
-                // there's a chance we'll be doing more than one search in different parts of the tree.
+                // the new start nodes. 
                 start_nodes = SearchTreeForNode(start_nodes, *query_part);
             }
-            else if (query_part->Type() == XPathQueryPart::QueryPartType::Parent)
+            else if (query_part->type() == typeid(XPathParentNode))
             {
                 // This part of the query selects the parent node. If the current node has no
                 // parent (i.e.- we're already at the root of the tree) then this is a no-op:
@@ -133,7 +171,7 @@
                         start_nodes.begin(),
                         start_nodes.end(),
                         [query_part](Node::Ptr n) -> bool {
-                            return ! query_part->Matches(n);
+                            return ! maybe_matches(n, *query_part);
                         }
                         ),
                     start_nodes.end()
@@ -144,7 +182,7 @@
             // next query part is not a parent node...
             auto next_query_part = query_part + 1;
             if (next_query_part != query_parts.cend()
-                && next_query_part->Type() != XPathQueryPart::QueryPartType::Parent)
+                && next_query_part->type() != typeid(XPathParentNode))
             {
                 NodeList new_start_nodes;
                 for (auto node: start_nodes)

=== modified file 'test/test_parser.cpp'
--- test/test_parser.cpp	2014-02-19 19:16:49 +0000
+++ test/test_parser.cpp	2014-07-28 19:46:11 +0000
@@ -100,6 +100,83 @@
     T expected_;
 };
 
+// Some helper functions to make dealing with these new node classes a little easier.
+
+void assert_is_specified_node(xpathselect::AnyNode const& node)
+{
+    ASSERT_EQ(node.type(), typeid(xpathselect::XPathSpecifiedNode));
+}
+
+void assert_is_wildcard_node(xpathselect::AnyNode const& node)
+{
+    ASSERT_EQ(node.type(), typeid(xpathselect::XPathWildcardNode));
+}
+
+void assert_is_parent_node(xpathselect::AnyNode const& node)
+{
+    ASSERT_EQ(node.type(), typeid(xpathselect::XPathParentNode));
+}
+
+void assert_is_search_node(xpathselect::AnyNode const& node)
+{
+    ASSERT_EQ(node.type(), typeid(xpathselect::XPathSearchNode));
+}
+
+// assert that the node is a specified node, and has the given name:
+void assert_node_is_specified_with_name(std::string const& name, xpathselect::AnyNode const& node)
+{
+    assert_is_specified_node(node);
+    ASSERT_EQ(name, boost::get<xpathselect::XPathSpecifiedNode>(node).names.at(0));
+}
+
+// assert that the node is a specified node and has no parameters:
+void assert_node_is_specified_with_num_parameters(int expected, xpathselect::AnyNode const& node)
+{
+    assert_is_specified_node(node);
+    ASSERT_EQ(expected, boost::get<xpathselect::XPathSpecifiedNode>(node).parameters.size());
+}
+
+void assert_node_is_wildcard_with_num_parameters(int expected, xpathselect::AnyNode const& node)
+{
+    assert_is_wildcard_node(node);
+    ASSERT_EQ(expected, boost::get<xpathselect::XPathWildcardNode>(node).parameters.size());
+}
+
+// assert that the parameter on 'node' at index 'index' has name and value equal to 'name' and 'val'
+template <typename T>
+void assert_parameter_name_and_value(
+    xpathselect::AnyNode const& node, 
+    int index, 
+    std::string const& name, 
+    variant_equality_assertion<T> const& val
+)
+{
+    if (node.type() == typeid(xpathselect::XPathSpecifiedNode))
+    {
+        auto n = boost::get<xpathselect::XPathSpecifiedNode>(node);
+        ASSERT_EQ("name", n.parameters.at(index).param_name);
+        boost::apply_visitor(
+            val,
+            n.parameters.at(0).param_value
+        );
+    }
+    else if (node.type() == typeid(xpathselect::XPathWildcardNode))
+    {
+        auto n = boost::get<xpathselect::XPathWildcardNode>(node);
+        ASSERT_EQ("name", n.parameters.at(index).param_name);
+        boost::apply_visitor(
+            val,
+            n.parameters.at(0).param_value
+        );
+
+    }
+    else
+    {
+        FAIL() << "Node is not specified or wildcard node, and thus does not have parameters.";
+    }
+}
+
+
 //////////////////////////////////////
 // Tests for basic type support:
 //////////////////////////////////////
@@ -333,6 +410,55 @@
                             std::pair<std::string, bool>("..", false)
                          ));
 
+// Test node name lists. 
+TEST(TestNodeNameList, test_single_name)
+{
+    xpathselect::NodeNameList result;
+    parser::xpath_grammar<std::string::iterator> g;
+
+    ASSERT_TRUE(test_parser_attr("someNode", g.spec_node_name_list, result));
+    ASSERT_EQ(1, result.size());
+    ASSERT_EQ("someNode", result.at(0));
+}
+
+TEST(TestNodeNameList, test_two_nodes)
+{
+    xpathselect::NodeNameList result;
+    parser::xpath_grammar<std::string::iterator> g;
+
+    ASSERT_TRUE(test_parser_attr("someNode,someOtherNode", g.spec_node_name_list, result));
+    ASSERT_EQ(2, result.size());
+    ASSERT_EQ("someNode", result.at(0));
+    ASSERT_EQ("someOtherNode", result.at(1));
+}
+
+TEST(TestNodeNameList, test_empty_string)
+{
+    xpathselect::NodeNameList result;
+    parser::xpath_grammar<std::string::iterator> g;
+
+    ASSERT_FALSE(test_parser_attr("", g.spec_node_name_list, result));
+}
+
+
+TEST(TestNodeNameList, test_comma_string)
+{
+    xpathselect::NodeNameList result;
+    parser::xpath_grammar<std::string::iterator> g;
+
+    ASSERT_FALSE(test_parser_attr(",", g.spec_node_name_list, result));
+}
+
+
+TEST(TestNodeNameList, test_wildcard_not_allowed_in_list)
+{
+    xpathselect::NodeNameList result;
+    parser::xpath_grammar<std::string::iterator> g;
+
+    ASSERT_FALSE(test_parser_attr("foo,*", g.spec_node_name_list, result));
+}
+
+
 /// Tests for parameter values. This test is much larger than it should be, since it seems to be
 // impossible to parameterise tests for both type and value. The solution I use here is to have
 // the actual test in a base class template method, and have several derive classes use different
@@ -480,8 +606,6 @@
 
     std::string result;
     ASSERT_EQ( expect_pass,  test_parser_attr(input, g.wildcard_node_name, result) );
-    if (expect_pass)
-        ASSERT_EQ(input, result);
 }
 
 INSTANTIATE_TEST_CASE_P(BasicNodeNames,
@@ -597,14 +721,14 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
+    xpathselect::XPathSpecifiedNode result;
     ASSERT_EQ( true,  test_parser_attr(input, g.spec_node, result) );
-    ASSERT_EQ("node_name", result.node_name_);
-    ASSERT_FALSE(result.parameter.empty());
-    ASSERT_EQ("param_name", result.parameter.at(0).param_name);
+    ASSERT_EQ("node_name", result.names.at(0));
+    ASSERT_FALSE(result.parameters.empty());
+    ASSERT_EQ("param_name", result.parameters.at(0).param_name);
     boost::apply_visitor(
         variant_equality_assertion<int>(123),
-        result.parameter.at(0).param_value
+        result.parameters.at(0).param_value
         );
 }
 
@@ -614,10 +738,10 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
+    xpathselect::XPathSpecifiedNode result;
     ASSERT_EQ( true,  test_parser_attr(input, g.spec_node, result) );
-    ASSERT_EQ("node_name", result.node_name_);
-    ASSERT_TRUE(result.parameter.empty());
+    ASSERT_EQ("node_name", result.names.at(0));
+    ASSERT_TRUE(result.parameters.empty());
 }
 
 TEST(TestXPathParser, test_wildcard_node)
@@ -626,10 +750,9 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
+    xpathselect::XPathWildcardNode result;
     ASSERT_EQ( true,  test_parser_attr(input, g.wildcard_node, result) );
-    ASSERT_EQ("*", result.node_name_);
-    ASSERT_TRUE(result.parameter.empty());
+    ASSERT_TRUE(result.parameters.empty());
 }
 
 TEST(TestXPathParser, test_wildcard_node_rejects_parameters)
@@ -638,7 +761,7 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
+    xpathselect::XPathWildcardNode result;
     ASSERT_FALSE( test_parser_attr(input, g.wildcard_node, result) );
 }
 
@@ -648,15 +771,14 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
+    xpathselect::XPathWildcardNode result;
     ASSERT_EQ( true,  test_parser_attr(input, g.wildcard_node_with_params, result) );
-    ASSERT_EQ("*", result.node_name_);
-    ASSERT_FALSE(result.parameter.empty());
-    ASSERT_EQ("param_name", result.parameter.at(0).param_name);
+    ASSERT_FALSE(result.parameters.empty());
+    ASSERT_EQ("param_name", result.parameters.at(0).param_name);
     boost::apply_visitor(
         variant_equality_assertion<int>(123),
-        result.parameter.at(0).param_value
-        );
+        result.parameters.at(0).param_value
+    );
 }
 
 
@@ -666,16 +788,16 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
-    ASSERT_EQ( true,  test_parser_attr(input, g.node, result) );
-    ASSERT_EQ( "*", result.node_name_ );
-    ASSERT_EQ( xpathselect::XPathQueryPart::QueryPartType::Normal, result.Type() );
-    ASSERT_EQ( 1, result.parameter.size() );
-    ASSERT_EQ( "name", result.parameter.at(0).param_name );
-    boost::apply_visitor(
-        variant_equality_assertion<std::string>("value"),
-        result.parameter.at(0).param_value
-        );
+    xpathselect::AnyNode result;
+    ASSERT_EQ( true,  test_parser_attr(input, g.any_node, result) );
+    
+    assert_node_is_wildcard_with_num_parameters(1, result);
+    assert_parameter_name_and_value(
+        result, 
+        0, 
+        "name", 
+        variant_equality_assertion<std::string>("value")
+    );
 }
 
 TEST(TestXPathParser, test_node_can_be_a_wildcard_node_without_params)
@@ -684,10 +806,9 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
-    ASSERT_EQ( true,  test_parser_attr(input, g.node, result) );
-    ASSERT_EQ( "*", result.node_name_ );
-    ASSERT_EQ( xpathselect::XPathQueryPart::QueryPartType::Normal, result.Type() );
+    xpathselect::AnyNode result;
+    ASSERT_EQ( true,  test_parser_attr(input, g.any_node, result) );
+    assert_is_wildcard_node(result);
 }
 
 TEST(TestXPathParser, test_node_can_be_a_spec_node_with_params)
@@ -696,16 +817,16 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
-    ASSERT_EQ( true,  test_parser_attr(input, g.node, result) );
-    ASSERT_EQ( "foo", result.node_name_ );
-    ASSERT_EQ( xpathselect::XPathQueryPart::QueryPartType::Normal, result.Type() );
-    ASSERT_EQ( 1, result.parameter.size() );
-    ASSERT_EQ( "name", result.parameter.at(0).param_name );
-    boost::apply_visitor(
-        variant_equality_assertion<std::string>("value"),
-        result.parameter.at(0).param_value
-        );
+    xpathselect::AnyNode result;
+    ASSERT_EQ( true,  test_parser_attr(input, g.any_node, result) );
+    
+    assert_node_is_specified_with_name("foo", result);
+    assert_parameter_name_and_value(
+        result,
+        0,
+        "name",
+        variant_equality_assertion<std::string>("value")
+    );
 }
 
 TEST(TestXPathParser, test_node_can_be_a_spec_node_without_params)
@@ -714,10 +835,10 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
-    ASSERT_EQ( true,  test_parser_attr(input, g.node, result) );
-    ASSERT_EQ( "foo", result.node_name_ );
-    ASSERT_EQ( xpathselect::XPathQueryPart::QueryPartType::Normal, result.Type() );
+    xpathselect::AnyNode result;
+    ASSERT_EQ( true,  test_parser_attr(input, g.any_node, result) );
+    
+    assert_node_is_specified_with_name("foo", result);
 }
 
 TEST(TestXPathParser, test_node_can_be_a_parent_node)
@@ -726,10 +847,9 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
-    ASSERT_EQ( true,  test_parser_attr(input, g.node, result) );
-    ASSERT_EQ( "..", result.node_name_ );
-    ASSERT_EQ( xpathselect::XPathQueryPart::QueryPartType::Parent, result.Type() );
+    xpathselect::AnyNode result;
+    ASSERT_EQ( true,  test_parser_attr(input, g.any_node, result) );
+    assert_is_parent_node(result);
 }
 
 TEST(TestXPathParser, test_search_node_followed_by_normal_node)
@@ -738,12 +858,11 @@
     // to give it some more data, even though we're not actually matching it.
     std::string input("//node_name");
     parser::xpath_grammar<std::string::iterator> g;
-    xpathselect::XPathQueryPart result;
+    xpathselect::XPathSearchNode result;
 
     // however, this means we can't use the test_parser_attr function, since it
     // returns false on a partial match. Use the parse(...) function directly:
     ASSERT_TRUE( parse(input.begin(), input.end(),g.search_node, result) );
-    ASSERT_TRUE( result.Type() == xpathselect::XPathQueryPart::QueryPartType::Search );
 }
 
 TEST(TestXPathParser, test_search_node_followed_by_wildcard_node_with_parameters)
@@ -752,12 +871,11 @@
     // to give it some more data, even though we're not actually matching it.
     std::string input("//*[foo=\"bar\"]");
     parser::xpath_grammar<std::string::iterator> g;
-    xpathselect::XPathQueryPart result;
+    xpathselect::XPathSearchNode result;
 
     // however, this means we can't use the test_parser_attr function, since it
     // returns false on a partial match. Use the parse(...) function directly:
     ASSERT_TRUE( parse(input.begin(), input.end(),g.search_node, result) );
-    ASSERT_TRUE( result.Type() == xpathselect::XPathQueryPart::QueryPartType::Search );
 }
 
 TEST(TestXPathParser, test_search_node_cannot_have_parameters)
@@ -766,9 +884,20 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
-    ASSERT_FALSE( test_parser_attr(input, g.search_node, result) );
-}
+    xpathselect::XPathSearchNode result;
+    ASSERT_FALSE( test_parser_attr(input, g.search_node, result) );
+}
+
+TEST(TestXPathParser, test_cannot_give_two_search_nodes)
+{
+    std::string input("////");
+
+    parser::xpath_grammar<std::string::iterator> g;
+
+    xpathselect::XPathSearchNode result;
+    ASSERT_FALSE( test_parser_attr(input, g.search_node, result) );
+}
+
 
 TEST(TestXPathParser, test_parent_node)
 {
@@ -776,9 +905,8 @@
 
     parser::xpath_grammar<std::string::iterator> g;
 
-    xpathselect::XPathQueryPart result;
+    xpathselect::XPathParentNode result;
     ASSERT_TRUE( test_parser_attr(input, g.parent_node, result) );
-    ASSERT_TRUE( result.Type() == xpathselect::XPathQueryPart::QueryPartType::Parent );
 }
 
 TEST(TestXPathParser, test_normal_sep_works)
@@ -801,123 +929,87 @@
 
 TEST(TestXPathParser, test_can_extract_query_list)
 {
-    std::string input("/node1/node2");
-
-    parser::xpath_grammar<std::string::iterator> g;
-
-    xpathselect::QueryList result;
-    ASSERT_TRUE(test_parser_attr(input, g.node_sequence, result));
+    xpathselect::QueryList result = xpathselect::parser::parse_query("/node1/node2");
     ASSERT_EQ(2, result.size());
-    ASSERT_EQ("node1", result.at(0).node_name_);
-    ASSERT_TRUE(result.at(0).parameter.empty());
-    ASSERT_EQ("node2", result.at(1).node_name_);
-    ASSERT_TRUE(result.at(1).parameter.empty());
+    assert_node_is_specified_with_name("node1", result.at(0));
+    assert_node_is_specified_with_num_parameters(0, result.at(0));
+    assert_node_is_specified_with_name("node2", result.at(1));
+    assert_node_is_specified_with_num_parameters(0, result.at(1));
 }
 
 TEST(TestXPathParser, test_can_extract_query_list_with_search)
 {
-    std::string input("//node1");
-
-    parser::xpath_grammar<std::string::iterator> g;
-
-    xpathselect::QueryList result;
-    ASSERT_TRUE(test_parser_attr(input, g.node_sequence, result));
+    xpathselect::QueryList result = xpathselect::parser::parse_query("//node1");
     ASSERT_EQ(2, result.size());
-    ASSERT_TRUE(result.at(0).Type() == xpathselect::XPathQueryPart::QueryPartType::Search );
-    ASSERT_TRUE(result.at(1).Type() == xpathselect::XPathQueryPart::QueryPartType::Normal );
-    ASSERT_TRUE(result.at(0).parameter.empty());
-    ASSERT_EQ("node1", result.at(1).node_name_);
-    ASSERT_TRUE(result.at(1).parameter.empty());
+    assert_is_search_node(result.at(0));
+    assert_node_is_specified_with_name("node1", result.at(1));
+    assert_node_is_specified_with_num_parameters(0, result.at(1));
 }
 
 TEST(TestXPathParser, test_mix_search_and_normal)
 {
-    std::string input("/node1//node2");
-
-    parser::xpath_grammar<std::string::iterator> g;
-
-    xpathselect::QueryList result;
-    ASSERT_TRUE(test_parser_attr(input, g.node_sequence, result));
+    xpathselect::QueryList result = xpathselect::parser::parse_query("/node1//node2");
 
     ASSERT_EQ(3, result.size());
 
-    ASSERT_EQ("node1", result.at(0).node_name_);
-    ASSERT_TRUE(result.at(0).Type() == xpathselect::XPathQueryPart::QueryPartType::Normal );
-    ASSERT_TRUE(result.at(0).parameter.empty());
-
-    ASSERT_TRUE(result.at(1).Type() == xpathselect::XPathQueryPart::QueryPartType::Search );
-    ASSERT_TRUE(result.at(1).parameter.empty());
-
-    ASSERT_EQ("node2", result.at(2).node_name_);
-    ASSERT_TRUE(result.at(2).Type() == xpathselect::XPathQueryPart::QueryPartType::Normal );
-    ASSERT_TRUE(result.at(2).parameter.empty());
+    assert_node_is_specified_with_name("node1", result.at(0));
+    assert_node_is_specified_with_num_parameters(0, result.at(0));
+
+    assert_is_search_node(result.at(1));
+
+    assert_node_is_specified_with_name("node2", result.at(2));
+    assert_node_is_specified_with_num_parameters(0, result.at(2));
 }
 
 TEST(TestXPathParser, test_mix_search_and_long_normal)
 {
-    std::string input("/node1//node2[name=\"val\"]/node3");
-
-    parser::xpath_grammar<std::string::iterator> g;
-
-    xpathselect::QueryList result;
-    ASSERT_TRUE(test_parser_attr(input, g.node_sequence, result));
-
-    ASSERT_EQ(4, result.size());
-
-    ASSERT_EQ("node1", result.at(0).node_name_);
-    ASSERT_TRUE(result.at(0).parameter.empty());
-
-    ASSERT_TRUE(result.at(1).Type() == xpathselect::XPathQueryPart::QueryPartType::Search );
-    ASSERT_TRUE(result.at(1).parameter.empty());
-
-    ASSERT_EQ("node2", result.at(2).node_name_);
-    ASSERT_EQ(1, result.at(2).parameter.size());
-    ASSERT_EQ("name", result.at(2).parameter.at(0).param_name);
-    boost::apply_visitor(
-        variant_equality_assertion<std::string>("val"),
-        result.at(2).parameter.at(0).param_value
-        );
-    ASSERT_EQ("node3", result.at(3).node_name_);
-    ASSERT_TRUE(result.at(3).parameter.empty());
+    xpathselect::QueryList result = xpathselect::parser::parse_query(
+        "/node1//node2[name=\"val\"]/node3"
+    );
+
+    EXPECT_EQ(4, result.size());
+
+    assert_node_is_specified_with_name("node1", result.at(0));
+    assert_node_is_specified_with_num_parameters(0, result.at(0));
+
+    assert_is_search_node(result.at(1));
+
+    assert_node_is_specified_with_name("node2", result.at(2));
+    assert_node_is_specified_with_num_parameters(1, result.at(2));
+    assert_parameter_name_and_value(
+        result.at(2), 
+        0, 
+        "name", 
+        variant_equality_assertion<std::string>("val")
+    );    
+    
+    assert_node_is_specified_with_name("node3", result.at(3));
+    assert_node_is_specified_with_num_parameters(0, result.at(3));
 }
 
 TEST(TestXPathParser, test_mix_normal_and_parent)
 {
-    std::string input("/node1/..");
-
-    parser::xpath_grammar<std::string::iterator> g;
-
-    xpathselect::QueryList result;
-    ASSERT_TRUE(test_parser_attr(input, g.node_sequence, result));
+    xpathselect::QueryList result = xpathselect::parser::parse_query("/node1/..");
 
     ASSERT_EQ(2, result.size());
 
-    ASSERT_EQ("node1", result.at(0).node_name_);
-    ASSERT_TRUE(result.at(0).parameter.empty());
+    assert_node_is_specified_with_name("node1", result.at(0));
+    assert_node_is_specified_with_num_parameters(0, result.at(0));
 
-    ASSERT_TRUE(result.at(1).Type() == xpathselect::XPathQueryPart::QueryPartType::Parent );
-    ASSERT_TRUE(result.at(1).parameter.empty());
+    assert_is_parent_node(result.at(1));
 }
 
 TEST(TestXPathParser, test_mix_normal_and_parent_and_wildcard)
 {
-    std::string input("/node1/../*");
-
-    parser::xpath_grammar<std::string::iterator> g;
-
-    xpathselect::QueryList result;
-    ASSERT_TRUE(test_parser_attr(input, g.node_sequence, result));
+    xpathselect::QueryList result = xpathselect::parser::parse_query("/node1/../*");
 
     ASSERT_EQ(3, result.size());
 
-    ASSERT_EQ("node1", result.at(0).node_name_);
-    ASSERT_TRUE(result.at(0).parameter.empty());
-
-    ASSERT_TRUE(result.at(1).Type() == xpathselect::XPathQueryPart::QueryPartType::Parent );
-    ASSERT_TRUE(result.at(1).parameter.empty());
-
-    ASSERT_TRUE(result.at(2).node_name_ == "*" );
-    ASSERT_TRUE(result.at(2).parameter.empty());
+    assert_node_is_specified_with_name("node1", result.at(0));
+    assert_node_is_specified_with_num_parameters(0, result.at(0));
+
+    assert_is_parent_node(result.at(1));
+    assert_is_wildcard_node(result.at(2));
 }
 
 class TestXPathParserQueryStrings : public ::testing::TestWithParam<std::pair<std::string, bool> >
@@ -930,10 +1022,9 @@
     std::string input = p.first;
     bool expect_pass = p.second;
 
-    parser::xpath_grammar<std::string::iterator> g;
-
-    xpathselect::QueryList result;
-    ASSERT_EQ( expect_pass,  test_parser_attr(input, g, result) );
+    bool actual_pass;
+    xpathselect::QueryList result = xpathselect::parser::parse_query(input, actual_pass);
+    ASSERT_EQ( expect_pass, actual_pass );
 }
 
 INSTANTIATE_TEST_CASE_P(BasicNodeNames,

=== modified file 'test/test_xpath_tree.cpp'
--- test/test_xpath_tree.cpp	2013-09-03 03:28:14 +0000
+++ test/test_xpath_tree.cpp	2014-07-28 19:46:11 +0000
@@ -47,6 +47,16 @@
     NodePtr leaf_2_;
 };
 
+std::string dump_result(xpathselect::NodeVector result)
+{
+    // horribly inneficient, but only called when tests fail, so... *shrug*
+    std::string s;
+    for (auto r: result)
+    {
+        s += r->GetName() + " ";
+    }
+    return s;
+}
 TEST_F(TestTreeFixture, test_simple)
 {
     xpathselect::NodeVector result = xpathselect::SelectNodes(root_, "/");
@@ -64,6 +74,17 @@
     ASSERT_EQ(expected, actual);
 }
 
+TEST_F(TestTreeFixture, test_multi_name_absolute)
+{
+    xpathselect::NodeVector result = xpathselect::SelectNodes(root_, "/Root/ChildLeft1,ChildRight1");
+    ASSERT_EQ(2, result.size());
+
+    std::set<xpathselect::Node::Ptr> to_be_matched { child_l1_, child_r1_ };
+    std::set<xpathselect::Node::Ptr> resultset(result.begin(), result.end());
+    
+    ASSERT_EQ(to_be_matched, resultset);
+}
+
 TEST_F(TestTreeFixture, test_simple_relative)
 {
     xpathselect::NodeVector result = xpathselect::SelectNodes(root_, "//ChildRight1");
@@ -175,7 +196,7 @@
 TEST_F(TestTreeFixture, test_wildcard)
 {
     xpathselect::NodeVector result = xpathselect::SelectNodes(root_, "/Root/*");
-    ASSERT_EQ(2, result.size());
+    ASSERT_EQ(2, result.size()) << dump_result(result);
     for(auto n : result)
     {
         ASSERT_TRUE(n == child_l1_ || n == child_r1_ );