Conditional parsing, non-standard approach

Question

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<bool(const string&)> &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;
 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" );
 }
 }
 previous_was_operator = true;
 }
 else if( !previous_was_operator )
 {
 syntax_error( "In a conditional all CONFIG strings must be seperated by a conditional operator \'+\', \'|\', \')\', or \'(\'." );
 break;
 }
 else // previous was operator, so now we have a CONFIG string
 {
 // check CONFIG string, and perform a logical OR
 // TODO: check effect of "!"
 current = current || config_contains(token);
 }
 }
 return result;
}

next_token() reads the next token, and returns false if this fails. syntax_error sets a global error status with message which is checked after calling this function. map_value returns true if the token can be converted to an enum class value (so if it is a conditional operator as defined by me). This algorithm is more flexible than a shunting yard (due to my future plans).

I have not tested this extremely well, but what do you think?

Answer 1

A few comments:

bool nectar_loader::resolve_conditional( const std::function<bool(const string&)> &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 <class func> 
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?