Conditional parsing, non-standard approach

Conditional parsing (aka read some tokens and return true or false).

As a solution to this SO question, I wrote following code:

```
bool nectar_loader::resolve_conditional( const std::function &config_contains )
{
bool result = true;
// My very own recursive implementation
/*
- each set of parenthesis is handled recursively
- logical AND: +
- logical OR: |
- logical NOT: !
- two bools: "result" and "current"
- "result" keeps global result, and is modified by "+"
- "current" keeps results for "|" and "!"
- syntax checking for invalid
*/
bool previous_was_operator = true;
bool current = false; // keep track of current state
string token;
while( next_token(token) )
{
conditional_operator op;
if( map_value(conditional_operator_map, token, op) )
{
if( previous_was_operator )
{
if( op == conditional_operator::not_op )
current ^= current; // negate next
else
{
syntax_error( "Conditional operators \'+\', \'|\', \')\', and \'(\' must be followed by a CONFIG string." );
break;
}
}
else
{
switch(op)
{
case conditional_operator::right_parenthesis:
// TODO: allow conditionals without outer parenthesis of the form (a+b)|(c)
return result;
case conditional_operator::left_parenthesis: // recurse
return resolve_conditional( config_contains );
case conditional_operator::plus_op:
result = result && current; // "current" -> "result"
if( !result ) // negative when an element of a "+" expression
current = false; // reset "current"
break;

A few comments:

bool nectar_loader::resolve_conditional( const std::function &config_contains )

I'd generally prefer to turn this into a template, and pass the correct function type (and if I used std::function at all, use it only internally to store something):

template  
bool nextar_loader::resolve_conditional(func &config_contains) {

    bool result = true;
    // My very own recursive implementation
    /*
      - each set of parenthesis is handled recursively
      - logical AND: +
      - logical OR:  |
      - logical NOT: !
      - two bools: "result" and "current"
      - "result" keeps global result, and is modified by "+"
      - "current" keeps results for "|" and "!"
      - syntax checking for invalid
    */

I'd prefer to move the block comment to just above the function header. I'd also prefer more descriptive names than result and current. Given their use, I'd consider names more like "expression_value" and "subexpression_value".

bool previous_was_operator = true;
    bool current = false; // keep track of current state

This comment seems to add nothing to our understanding of the code. I'd consider something more like: "// Assume current sub-expression is invalid until parsed." (assuming that's really why you've initialized it to false).

string token;
    while( next_token(token) )
    {
        conditional_operator op;
        if( map_value(conditional_operator_map, token, op) )
        {
            if( previous_was_operator )
            {
                if( op == conditional_operator::not_op )
                    current ^= current; // negate next

Again, the comment seems to do little to assist understanding of the code. I'd probably start by pointing out that an expression like a op1 op2 b is valid if and only if op2 is a unary operator (of which this grammar apparently only supports one).

case conditional_operator::plus_op:
                        result = result && current; // "current" -> "result"

Given that you're using it as an and, I'd name this conditional_operator::and_op (or something similar). Then name should reflect the logical meaning of the operator, not the fact that it happens to be (mis-)represented with the + character.

if( !result ) // negative when an element of a "+" expression
                        current = false; // reset "current"

This if statement appears redundant (at least to me). You've just set result to the logical and of current and result, so it will be true if and only if both inputs were already true.

result = current = (current && result);

So, both end up true if and only if both started out true. If either started out false, both end up false.

break;
                    case conditional_operator::or_op: // combine with "current"

                    case conditional_operator::not_op: // unreachable
                        throw runtime_error( "Internal logic error in nectar_loader::resolve_conditional" );

This looks rather like a bug. Did you really intend that or_op would throw an exception as unreachable, or should there be a break after the or_op case?

StackExchange Code Review Q#2914, answer score: 2