Calculator/math expression parser (supporting complex numbers)

Question

I would love some honest feedback on my latest project. I've included the part I'm guessing is most interesting, but the entire code can be found on github: https://github.com/Raincode/DeskCalc or there is a gist with most of the code: https://gist.github.com/Raincode/e5115669bb647ceba690252ed39445f8 (includes the grammar I tried to implement)

The code is based on the calculator from "The C++ Programming Language" by Stroustrup. Here is a general orientation of the code layout:

TokenStream does the lexical-analysis

Parser recursive descent parser which interprets the tokens and calculates the result on the fly.

SymbolTable stores variables, functions and lists. In SymbolTable.cpp I use some makros which I find particularly unelegant.

Calculator is the driver which uses the Parser etc. to read input from a console window and display the results.

Function stores the function term as a string. When called, it sets its arguments as temporary variables and lets a parser compute the result from the string. Probably this is inefficient, because it parses the function term over and over again. However, I'm not using an AST so this is the best solution I could come up with.

SymbolGuard is used for implementing functions. Used for setting the function arguments as temporary variables in the symbol table.

Parser.cpp

#include "Parser.hpp"
#include <cmath>
#include <iostream>
#include "mps/stream_util.hpp"
#include "math_util.hpp"
#include "SymbolTable.hpp"
Parser::Parser(SymbolTable& table)
 : table{ table } { }
void Parser::set_symbol_table(SymbolTable& t)
{
 table = t;
}
void Parser::parse(std::istream& is)
{
 ts.set_input(is);
 parse();
}
void Parser::parse(const std::string& str)
{
 ts.set_input(str);
 parse();
}
void Parser::parse()
{
 ts.get();
 while (!consume(Kind::End))
 stmt();
}
void Parser::stmt()
{
 hasResult = true;
 if (peek(Kind::FuncDef))
 func_def();
 else if (peek(Kind::Delete))
 deletion();
 else if (!peek(Kind::Print))
 res = expr();
 if (hasResult && onRes)
 onRes(res);
 if (!peek(Kind::End))
 expect(Kind::Print);
}
void Parser::func_def()
{
 expect(Kind::FuncDef);
 Function func{ ident(), table };
 parse_param_list(func);
 expect(Kind::Assign);
 parse_func_term(func);
 List test_args(func.numArgs(), 1);
 func(test_args);
 table.set_func(func.name(), func);
 hasResult = false;
}
void Parser::parse_param_list(Function& func)
{
 expect(Kind::LParen);
 do {
 func.add_var(ident());
 } while (consume(Kind::Comma));
 expect(Kind::RParen);
}
void Parser::parse_func_term(Function& func)
{
 std::ostringstream term;
 while (!peek(Kind::Print) && !peek(Kind::End) && !peek(Kind::RBracket)) {
 term << ts.current();
 ts.get();
 }
 func.set_term(term.str());
}
void Parser::deletion()
{
 expect(Kind::Delete);
 if (!peek(Kind::String))
 error("No arguments provided to delete");
 do {
 del_symbol();
 } while (peek(Kind::String));
 hasResult = false;
}
static bool is_const(const std::string& var)
{
 return var == "pi" || var == "e" || var == "i" || var == "deg";
}
void Parser::del_symbol()
{
 auto& name = ident();
 if (is_const(name))
 error(name, " is a constant");
 if (table.is_reserved_func(name))
 error(name, " is a built-in function");
 table.remove_symbol(name);
}
void Parser::list_def(const std::string& name)
{
 table.set_list(name, list());
}
Complex Parser::expr()
{
 auto left = term();
 for (;;) {
 if (consume(Kind::Plus)) 
 left += term();
 else if (consume(Kind::Minus)) 
 left -= term();
 else
 return left;
 }
}
Complex Parser::term()
{
 auto left = sign();
 for (;;) {
 if (consume(Kind::Mul))
 left *= sign();
 else if (consume(Kind::Div))
 left = safe_div(left, sign());
 else if (consume(Kind::FloorDiv))
 left = safe_floordiv(left, sign());
 else if (consume(Kind::Mod))
 left = safe_mod(left, sign());
 else if (consume(Kind::Parallel))
 left = impedance_parallel(left, sign());
 else 
 return left;
 }
}
Complex Parser::sign()
{
 if (consume(Kind::Minus)) 
 return -postfix();
 consume(Kind::Plus);
 return postfix();
}
Complex Parser::postfix()
{
 auto left = prim();
 for (;;) {
 if (consume(Kind::Pow))
 return pretty_pow(left, sign());
 else if (peek(Kind::String))
 return left * postfix();
 else if (peek(Kind::LParen))
 return left * prim();
 else if (consume(Kind::Fac))
 left = factorial(left);
 else
 return left;
 }
}
Complex Parser::prim()
{
 if (consume(Kind::Number)) 
 return prevTok.num;
 if (peek(Kind::String))
 return resolve_str_tok();
 if (consume(Kind::LParen)) {
 auto val = expr();
 expect(Kind::RParen);
 return val;
 }
 error("Unexpected Token ", ts.current());
 throw std::logic_error{ "Fall through prim()" }; // silence C4715
}
Complex Parser::resolve_str_tok()
{
 auto name = ident();
 if (peek(Kind::LParen))
 return table.call_func(name, arg_list());
 else if (consume(Kind::Assign)) {
 if (peek(Kind::LBracket)) {
 list_def(name);
 return no_result();
 }
 return var_def(name);
 }
 else if (table.has_list(name)) {
 if (!peek(Kind::Print) && !peek(Kind::End))
 error("Unexpected Token ", ts.current());
 print_list(std::cout, table.list(name));
 std::cout << '\n';
 return no_result();
 }
 return table.value_of(name);
}
Complex Parser::var_def(const std::string& name)
{
 if (is_const(name))
 error("Cannot override constant ", name);
 if (peek(Kind::LBracket)) {
 table.set_list(name, list());
 return no_result();
 }
 if (table.isset(name) && !table.has_var(name))
 error(name, " is already defined");
 auto val = expr();
 table.set_var(name, val);
 return varDefIsRes ? val : no_result();
}
Complex Parser::no_result()
{
 hasResult = false;
 return 0; // dummy value
}
List Parser::list()
{
 expect(Kind::LBracket);
 List l;
 if (consume(Kind::For)) { // [for var=start, end:step loopExpression]
 auto var = ident();
 expect(Kind::Assign);
 auto start = prim().real();
 expect(Kind::Comma);
 auto end = prim().real();
 double step{ 1 };
 if (consume(Kind::Colon))
 step = prim().real();
 Function f{ "__internal__", table };
 f.add_var(var);
 parse_func_term(f);
 if (start < end && step <= 0 || start > end && step >= 0)
 error("Infinite loop");
 for (auto i = start; i <= end; i += step)
 l.emplace_back(f({ Complex(i) }));
 }
 else
 l = list_elem();
 expect(Kind::RBracket);
 return l;
}
List Parser::arg_list()
{
 expect(Kind::LParen);
 List args;
 if (peek(Kind::String) && table.has_list(ts.current().str))
 args = table.list(ident());
 else if (peek(Kind::LBracket))
 args = list();
 else
 args = list_elem();
 expect(Kind::RParen);
 if (args.empty())
 error("Invalid empty argument list");
 return args;
}
List Parser::list_elem()
{
 List list;
 if (!peek(Kind::RParen) && !peek(Kind::RBracket)) {
 do {
 list.emplace_back(expr());
 } while (consume(Kind::Comma));
 }
 return list;
}
const std::string& Parser::ident()
{
 expect(Kind::String);
 return prevTok.str;
}
bool Parser::consume(Kind kind)
{
 if (ts.current().kind == kind) {
 prevTok = ts.current();
 ts.get();
 return true;
 }
 return false;
}
bool Parser::peek(Kind kind) const
{
 return ts.current().kind == kind;
}
void Parser::expect(Kind kind)
{
 if (!consume(kind))
 error("Expected Token ", kind);
}

Some of the features I added are:

• variables
• functions w/ multiple variables
• calculating standard deviation/uncertainty for a list of numbers
• factorial operator
• pow operator
• floor division operator
• modulo operator
• complex number arithmetic
• a few useful commands, which are executed by the Calculator, not by the parser.

Answer 1

Note: Visual Studio doesn't fully support every C++17/C++14/C++11 feature, so it is possible that some of the advice here doesn't compile.

Also, some things might be considered opinion based, but I think that they are not and help read the code better (at least for me and I think others too). Feel free to ignore them if you don't agree.

1. I'm not that experienced in platform dependent code, but I would say that you have to at least support Windows, macOS and some Linux distributions, to make it available to most people, only if possible of course.

2. std::endl is only needed if you need to flush the output stream, and even then, you should use std::flush to be explicit. Also, stderr is always automatically flushed, so no need for either std::endl or std::flush.

3. Have you heard of const correctness? Variables should be const if you do not intend to modify them:

   catch (std::runtime_error& e) // never going to modify e, mark it as const
   auto str = mps::str::to_string(parser.symbol_table().value_of("ans")); // same
   // ...
   

4. As you are using exceptions, this is even more needed: Mark functions that do not throw noexcept. This enables the compiler to optimize a bit more effectively, as it will know when functions might throw or not.

5. Mark classes which do not have a virtual destructor and thus should not be used as base classes as final.

6. Avoid std::function. It uses type-erasure, which is not very cheap. In your case use instead functions pointers.

7. Don't use using namespace std; for the reasons outlined here. In your case though, it seems that you do not have any possible name collisions, but defining a simple variable count will already fail to compile, so your choice. Also, you seem to mix and match putting std:: in front of types. Don't do that. Either you stick to the prefix, or not.

8. This

   if (ifstream ifs{ "intro.txt" })
   

   seems pretty sneaky. Be more explicit:

   if (ifstream ifs{ "intro.txt" }; ifs.is_open())
   

9. Try to reduce variable shadowing. In Calculator's constructor, there are 2 variables named ifs, which could cause confusion if the constructor grows sometime in the future. ifs is also not a very descriptive name. Consider using another name, like prompt_stream or something.

10. Instead of passing const std::string&, pass std::string_view instead. This avoids the copy when passing a string literal like "hello".

11. If you are only going to use a variable inside an if condition, define it inside:

    if (const auto found = commands.find(mps::str::tolower(cmd)); found != end(commands))
    

    instead of

    auto found = commands.find(mps::str::tolower(cmd));
    if (found != end(commands))
    

12. Don't create classes just for the sake of it. This isn't Java after all :) Warning really wants to be a function. Even better would be to implement some sort of logging, and use that.

13. #pragma once is not Standard C++, even if it is supported by every major compiler (VS, gcc, clang, ICC). #ifndef and #define are the way to go. But that's up to you.

14. You forgot some headers, like <utility> in ErrorReported (for std::forward).

15. Using forwarding references is better than taking const& in some cases involving templates. Sometimes, you followed this. But in others (ErrorReporter), you are not.

16. Why does ErrorReporter need an error function? You can implement everything using operator():

    template<class T, class ...Args>
    void operator()(T&& t, Args&&... args)
    {
     msgStream << t;
     (*this)(std::forward<Args>(args)...);
    }
    void operator()()
    {
     const auto msg = msgStream.str();
     msgStream.str("");
     msgStream.clear();
     throw std::runtime_error{ msg };
    }
    

17. Please don't use integers as if they were booleans:

    template<class T>
    T safe_mod(T left, T right)
    {
     if (!right) // 'right' is not a boolean!
     throw std::runtime_error{ "Divide by Zero" };
     return left % right;
    }
    

    It's clearer if you use if (right == 0) in my opinion.

18. auto is pretty useful, and you also use it sometimes. I would use it for things like auto pi = 3.14 too, but that's up to you.

19. You seem to want high precision floating point numbers. You can use a third party library, or if you don't want to, at least switch to long double and mark your constants to long double too:

    constexpr auto pi = 3.1415926535897932385l;
    

20. Why didn't I use aggregate initialization above? Because for ints, it will be deduced to std::initializer_list in older versions of C++. This is fixed in C++17 though, so you can safely ignore this.

21. Some functions in math_util take their parameters by value, even though there is no reason to. Pass them by const& instead.

22. I don't know about this, but I think you are only supposed to use the free functions std::begin, std::cbegin, and so one if you are writing generic code. If you are not, things like std::cbegin(list) should be list.cbegin() instead.

23. Instead of introducing a separate function to print complex numbers, why not overload operator<<:

    template<typename T>
    std::ostream& operator<<(std::ostream& os, const std::complex<T>& number) noexcept;
    

24. Taking advantage of move semantics:

    Function::Function(std::string name, SymbolTable& table)
     : funcName{ std::move(name) }, table{ table }
    

    If you pass a prvalue, then here, only 1 constructor and 1 move is taking place, which is more efficient then 1 constructor and 1 copy.

25. sep is only 1 character, so instead of defining it as a std::string, you can use a simple char.

26. Why are you using pointers in TokenStream? It's good that you deleted the copy constructors to avoid undefined behavior, you never actually free input, even if you allocate it yourself in one case only. That's why it is better to use references.

27. static functions outside of classes inside a TU is, well, "deprecated". Use anonymous namespaces instead.

28. I don't have a solution for the macros, as currently I think they are unavoidable given the design of your program. But, sometimes, macros are necessary, and this is one of them. I don't think they are inelegant though, you can always put them near the table if you don't want to see them :)

29. I think it is better if you create a new type for variables. That way, you can make is_const a property of a variable.

30. Because you are using another library, mps, I don't think you should include the tests for it, as that is not really important for your program.

31. I don't know if this is good practice or not, but it would be better and safer to return something at the end of every function, as that avoids undefined behavior, even if it is never reached.

• Deduplicator: Functions with static linkage were deprecated. They are no more. And he's returning a value on every path of his non-void-functions, so that's fine.
• Rakete1111: @Deduplicator That's why I used the quotes: Anonymous namespaces are better and should be used instead. Some return statements are missing where OP uses an infinite loop.
• Deduplicator: In what way are anonymous namespaces "better"? And if a loop is infinite, of course there shouldn't be any code following it.
• Rakete1111: @Deduplicator Yeah, I guess you're right :) I read it in some SO answer a while ago, but there was no reason mentioned. I'll try to find out again.
• smoothware: @Rakete1111 Thank you very much for your reply, I will take my time to look at every point and try to apply it. It's funny you mention the separate Variable Type - I used to have it and discarded it thinking "I won't need this until I have a const keyword". I never realized I didn't have a dtor in TokenStream, that's really bad... The reason I store a pointer is so I can have a default-ctor for Parser, which has a TokenStream member. (And I can do things like set_input(new std::istringstream("expression)); ) Anyways, thanks again I appreciate your time.