Skip to main content
added 53 characters in body
Source Link
Loki Astari
  • 96.6k
  • 5
  • 125
  • 338
     case '|': // | or ||
      case '|': // | or ||
            if (it[1] == '*')

##N#ew###New Style auto return I still prefer the old style of function declaration over the new style.

       case '-': // - (unary or binary)
        case '-': // - (unary or binary)
            if (res.empty())
            {
                // STUFF
                break
            }
            else
            {
                // STUFF
                break;
            }

Personally iI would havemove the breaks outside the if statement. So it is clear that the case statement isdoes have a break.

##Changes that may help: ###Limit of 16 parameters:

                   Number params[16];
                    Number params[16];
                    //  STUFF
                    operands.push(func(params));
                    break;

##New Stuff: ###Not seen this before callbacks.emplace( std::piecewise_construct, // Will have to go read about this. std::forward_as_tuple(name), std::forward_as_tuple(function) ); ###Would using move semantics help for future

       callbacks.emplace(
output            std::piecewise_construct,     // Will have to go read about this.push
            std::forward_as_tuple(operations.topname),
            std::forward_as_tuple(function)
        );

###Would using move semantics help for future

output.push(operations.top());

      case '|': // | or ||
            if (it[1] == '*')

##N#ew Style auto return I still prefer the old style of function declaration over the new style.

        case '-': // - (unary or binary)
            if (res.empty())
            {
                // STUFF
                break
            }
            else
            {
                // STUFF
                break;
            }

Personally i would have the breaks. So it is clear that the case statement is

##Changes that may help: ###Limit of 16 parameters:

                    Number params[16];
                    //  STUFF
                    operands.push(func(params));
                    break;

##New Stuff: ###Not seen this before callbacks.emplace( std::piecewise_construct, // Will have to go read about this. std::forward_as_tuple(name), std::forward_as_tuple(function) ); ###Would using move semantics help for future

output.push(operations.top());
     case '|': // | or ||
            if (it[1] == '*')

###New Style auto return I still prefer the old style of function declaration over the new style.

       case '-': // - (unary or binary)
            if (res.empty())
            {
                // STUFF
                break
            }
            else
            {
                // STUFF
                break;
            }

Personally I would move the breaks outside the if statement. So it is clear that the case statement does have a break.

##Changes that may help: ###Limit of 16 parameters

                   Number params[16];
                    //  STUFF
                    operands.push(func(params));
                    break;

##New Stuff: ###Not seen this before

       callbacks.emplace(
            std::piecewise_construct,     // Will have to go read about this.
            std::forward_as_tuple(name),
            std::forward_as_tuple(function)
        );

###Would using move semantics help for future

output.push(operations.top());

Source Link
Loki Astari
  • 96.6k
  • 5
  • 125
  • 338

##Suspected Bugs:

A typo I think

      case '|': // | or ||
            if (it[1] == '*')

##Stylistic differences: A couple of things I would do differently.
But I believe you and I differ on these minor stylistic things:

###User defined Types I prefer to use an initial caps when defining types. It helps distinguish types from objects.

##N#ew Style auto return I still prefer the old style of function declaration over the new style.

// old style
std::vector<token<Number>> tokenize(const std::string& expr)

// new style
auto tokenize(const std::string& expr) -> std::vector<token<Number>>

I use the new style only when the return type depends on the inputs.

###Lexer I would have used a lexer tool. That's how I roll.

Hard To spot break

        case '-': // - (unary or binary)
            if (res.empty())
            {
                // STUFF
                break
            }
            else
            {
                // STUFF
                break;
            }

Personally i would have the breaks. So it is clear that the case statement is

        case '-': // - (unary or binary)
            if (res.empty())
            {
                // STUFF
            }
            else
            {
                // STUFF
            }
            break;

Same thing in:

case '!': // ! (prefix or postfix) and !=

##Changes that may help: ###Limit of 16 parameters:

                    Number params[16];
                    //  STUFF
                    operands.push(func(params));
                    break;

Why not use a std::vector?
You can still pass it as an array pointer by taking the address of the first argument.

                    std::vector<Number> x;
                    //  STUFF
                    operands.push(func(&params[0]));
                    break;

##New Stuff: ###Not seen this before callbacks.emplace( std::piecewise_construct, // Will have to go read about this. std::forward_as_tuple(name), std::forward_as_tuple(function) ); ###Would using move semantics help for future

output.push(operations.top());

In the future it may be more than just numbers that you use in the expression. You want to prepare for that by using move semantics when transfering values in a couple of places.

output.push(std::move(operations.top()));