# Following in the Footsteps of Calculators: Qalculate\! Previously we did code reviews of large mathematical packages, for example, Scilab and Octave, whereby calculators remained aloof as small utilities, in which it is difficult to make errors due to their small codebase\. We were wrong that we haven't paid attention to them\. The case with posting the source code of the Windows calculator showed that actually everyone was interested in discussing types of errors hiding in it\. Moreover, the number of errors there was more than enough to write an article about that\. My colleagues and I, we decided to explore the code of a number of popular calculators, and it turned out that the code of the Windows calculator was not that bad \(spoiler\)\. ![0616_Qalculate/image1.png](https://import.viva64.com/docx/blog/0616_Qalculate/image1.png) ## Introduction [Qalculate\!](https://qalculate.github.io/) is a multi\-purpose cross\-platform desktop calculator\. It is simple to use but provides power and versatility normally reserved for complicated math packages, as well as useful tools for everyday needs \(such as currency conversion and percent calculation\)\. The project consists of two components: [libqalculate](https://github.com/Qalculate/libqalculate) \(library and CLI\) and [qalculate\-gtk](https://github.com/Qalculate/qalculate-gtk) \(GTK \+ UI\)\. The study involved only the libqalculate code\. To easily compare the project with Windows Calculator, which we have recently checked, I'm citing the output of the Cloc utility for libqalculate: ![0616_Qalculate/image2.png](https://import.viva64.com/docx/blog/0616_Qalculate/image2.png) Considering it subjectively, there are more errors in it and they are more critical than in the Windows calculator code\. Nevertheless, I would recommend making conclusions on your own, having read this code overview\. Bug reviews for the other projects: * [Counting Bugs in Windows Calculator](https://pvs-studio.com/en/blog/posts/cpp/0615/) * [Following in the Footsteps of Calculators: SpeedCrunch](https://pvs-studio.com/en/blog/posts/cpp/0618/) The analysis tool is the [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) static code analyzer\. It is a set of solutions for code quality control, search for bugs and potential vulnerabilities\. Supported languages include: C, C\+\+, C\# and Java\. You can run the analyzer on Windows, Linux and macOS\. ## Copy\-paste and Typos Again\! [V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. Number\.cc 4018 ```cpp bool Number::square() { .... if(mpfr_cmpabs(i_value->internalLowerFloat(), i_value->internalUpperFloat()) > 0) { mpfr_sqr(f_tmp, i_value->internalLowerFloat(), MPFR_RNDU); mpfr_sub(f_rl, f_rl, f_tmp, MPFR_RNDD); } else { mpfr_sqr(f_tmp, i_value->internalLowerFloat(), MPFR_RNDU); mpfr_sub(f_rl, f_rl, f_tmp, MPFR_RNDD); } .... } ``` The code is absolutely the same in the _if_ and _else _blocks_\._ Adjacent code fragments are very similar to this one, but different functions are used in them: _internalLowerFloat\(\)_ and _internalUpperFloat\(\)_\. It is safe to assume that a developer copied the code and forgot to correct the name of the function here\. [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions '\!mtr2\.number\(\)\.isReal\(\)' to the left and to the right of the '\|\|' operator\. BuiltinFunctions\.cc 6274 ```cpp int IntegrateFunction::calculate(....) { .... if(!mtr2.isNumber() || !mtr2.number().isReal() || !mtr.isNumber() || !mtr2.number().isReal()) b_unknown_precision = true; .... } ``` In this case, duplicated expressions appeared due to the fact that in one place _mtr2 _was written instead of_ mtr\. _ Thus, a call of the _mtr\.number\(\)\.isReal\(\)_ function is absent in the condition\. [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'vargs\[1\]\.representsNonPositive\(\)' to the left and to the right of the '\|\|' operator\. BuiltinFunctions\.cc 5785 ![0616_Qalculate/image3.png](https://import.viva64.com/docx/blog/0616_Qalculate/image3.png) We would have never found defects in this code manually\! But here there are\. Moreover, in the original file these fragments are written in a single line\. The analyzer has detected a duplicated expression _vargs\[1\]\.representsNonPositive\(\)_, which may indicate a typo or, consequently, a potential error\. Here's the entire list of suspicious places, which one can barely puzzle out\. * V501 There are identical sub\-expressions 'vargs\[1\]\.representsNonPositive\(\)' to the left and to the right of the '\|\|' operator\. BuiltinFunctions\.cc 5788 * V501 There are identical sub\-expressions 'append' to the left and to the right of the '&&' operator\. MathStructure\.cc 1780 * V501 There are identical sub\-expressions 'append' to the left and to the right of the '&&' operator\. MathStructure\.cc 2043 * V501 There are identical sub\-expressions '\(\* v\_subs\[v\_order\[1\]\]\)\.representsNegative\(true\)' to the left and to the right of the '&&' operator\. MathStructure\.cc 5569 ## Loop With Incorrect Condition [V534](https://pvs-studio.com/en/docs/warnings/v534/) It is likely that a wrong variable is being compared inside the 'for' operator\. Consider reviewing 'i'\. MathStructure\.cc 28741 ```cpp bool MathStructure::isolate_x_sub(....) { .... for(size_t i = 0; i < mvar->size(); i++) { if((*mvar)[i].contains(x_var)) { mvar2 = &(*mvar)[i]; if(mvar->isMultiplication()) { for(size_t i2 = 0; i < mvar2->size(); i2++) { if((*mvar2)[i2].contains(x_var)) {mvar2 = &(*mvar2)[i2]; break;} } } break; } } .... } ``` In the inner loop, the _i2 _variable represents a counter, but due to a typo an error was made \- the _i_ variable from the outer loop is used in the loop exit condition\. ## Redundancy or an Error? [V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting this expression\. The expression is excessive or contains a misprint\. Number\.cc 6564 ```cpp bool Number::add(const Number &o, MathOperation op) { .... if(i1 >= COMPARISON_RESULT_UNKNOWN && (i2 == COMPARISON_RESULT_UNKNOWN || i2 != COMPARISON_RESULT_LESS)) return false; .... } ``` 3 years ago after I got an eyeful of such code, I wrote a cheat sheet for me and other developers: "[Logical Expressions in C/C\+\+\. Mistakes Made by Professionals](https://pvs-studio.com/en/blog/posts/cpp/0390/)"\. When I come across such code, I make sure that the note hasn't become less relevant\. You can look into the article, find a pattern of the error corresponding to the code, and find out all the nuances\. In the case of this example, we'll go to the section "Expression \=\= \|\| \!\=" and find out that the expression _i2_ _\=\=_ _COMPARISON\_RESULT\_UNKNOWN_ affects nothing\. ## Dereferencing Unchecked Pointers [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'o\_data' pointer was utilized before it was verified against nullptr\. Check lines: 1108, 1112\. DataSet\.cc 1108 ```cpp string DataObjectArgument::subprintlong() const { string str = _("an object from"); str += " \""; str += o_data->title(); // <= str += "\""; DataPropertyIter it; DataProperty *o = NULL; if(o_data) { // <= o = o_data->getFirstProperty(&it); } .... } ``` In one function the _o\_data_ pointer is dereferenced both without and with a check\. This can be redundant code, or a potential error\. I'm leaning toward the latter\. There are two similar places: * V595 The 'o\_assumption' pointer was utilized before it was verified against nullptr\. Check lines: 229, 230\. Variable\.cc 229 * V595 The 'i\_value' pointer was utilized before it was verified against nullptr\. Check lines: 3412, 3427\. Number\.cc 3412 ## free\(\) or delete \[\]? [V611](https://pvs-studio.com/en/docs/warnings/v611/) The memory was allocated using 'new' operator but was released using the 'free' function\. Consider inspecting operation logics behind the 'remcopy' variable\. Number\.cc 8123 ```cpp string Number::print(....) const { .... while(!exact && precision2 > 0) { if(try_infinite_series) { remcopy = new mpz_t[1]; // <= mpz_init_set(*remcopy, remainder); } mpz_mul_si(remainder, remainder, base); mpz_tdiv_qr(remainder, remainder2, remainder, d); exact = (mpz_sgn(remainder2) == 0); if(!started) { started = (mpz_sgn(remainder) != 0); } if(started) { mpz_mul_si(num, num, base); mpz_add(num, num, remainder); } if(try_infinite_series) { if(started && first_rem_check == 0) { remainders.push_back(remcopy); } else { if(started) first_rem_check--; mpz_clear(*remcopy); free(remcopy); // <= } } .... } .... } ``` The memory for the _remcopy_ array is allocated and released in different ways, which is a serious error\. ## Lost Changes [V672](https://pvs-studio.com/en/docs/warnings/v672/) There is probably no need in creating the new 'm' variable here\. One of the function's arguments possesses the same name and this argument is a reference\. Check lines: 25600, 25626\. MathStructure\.cc 25626 ```cpp bool expand_partial_fractions(MathStructure &m, ....) { .... if(b_poly && !mquo.isZero()) { MathStructure m = mquo; if(!mrem.isZero()) { m += mrem; m.last() *= mtest[i]; m.childrenUpdated(); } expand_partial_fractions(m, eo, false); return true; } .... } ``` The _m_ variable in the function is passed by reference, which means its modification\. However, the analyzer has detected that the code contains the variable with the same name, which overlaps the scope of function's parameter, allowing for loss of changes\. ## Strange Pointers [V774](https://pvs-studio.com/en/docs/warnings/v774/) The 'cu' pointer was used after the memory was released\. Calculator\.cc 3595 ```cpp MathStructure Calculator::convertToBestUnit(....) { .... CompositeUnit *cu = new CompositeUnit("", "...."); cu->add(....); Unit *u = getBestUnit(cu, false, eo.local_currency_conversion); if(u == cu) { delete cu; // <= return mstruct_new; } delete cu; // <= if(eo.approximation == APPROXIMATION_EXACT && cu->hasApproximateRelationTo(u, true)) { // <= if(!u->isRegistered()) delete u; return mstruct_new; } .... } ``` The analyzer warns that the code calls a method of the _cu_ object right after deallocating memory\. But when trying to grapple with it, the code turns out to be even more strange\. Firstly, calling _delete cu_ happens always \- both in the condition and after that\. Secondly, the code after the condition implies that the pointers _u_ and _cu_ are not equal, which means that after deleting the _cu _object it is quite logical to use the _u_ object\. Most likely, a typo was made in the code and the author of the code wanted to use only the _u_ variable\. ## Usage of the find Function [V797](https://pvs-studio.com/en/docs/warnings/v797/) The 'find' function is used as if it returned a bool type\. The return value of the function should probably be compared with std::string::npos\. Unit\.cc 404 ```cpp MathStructure &AliasUnit::convertFromFirstBaseUnit(....) const { if(i_exp != 1) mexp /= i_exp; ParseOptions po; if(isApproximate() && suncertainty.empty() && precision() == -1) { if(sinverse.find(DOT) || svalue.find(DOT)) po.read_precision = READ_PRECISION_WHEN_DECIMALS; else po.read_precision = ALWAYS_READ_PRECISION; } .... } ``` Even though the code can be successfully compiled, it looks suspicious, as the _find _function returns the number of the type _std::string::size\_type_\. The condition will be true if the point is found in any part of the string except if the point is at the beginning\. It is a strange check\. I'm not sure but, perhaps, this code should be rewritten as follows: ```cpp if( sinverse.find(DOT) != std::string::npos || svalue.find(DOT) != std::string::npos) { po.read_precision = READ_PRECISION_WHEN_DECIMALS; } ``` ## Potential Memory Leak [V701](https://pvs-studio.com/en/docs/warnings/v701/) realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 'buffer' is lost\. Consider assigning realloc\(\) to a temporary pointer\. util\.cc 703 ```cpp char *utf8_strdown(const char *str, int l) { #ifdef HAVE_ICU .... outlength = length + 4; buffer = (char*) realloc(buffer, outlength * sizeof(char)); // <= .... #else return NULL; #endif } ``` When working with the _realloc\(\)_ function it is recommended to use an intermediate buffer, as in case if it is impossible to allocate memory, the pointer to the old memory area will be irretrievably lost\. ## Conclusion The Qalculate\! project tops the list of the best free calculators, whereas it contains many serious errors\. On the other hand, we haven't checked out its competitors yet\. We'll try to go over all popular calculators\. As for comparing with the quality of the calculator from the Windows world, the utility from Microsoft looks more reliable and well\-worked so far\. Check your own "Calculator" \- download [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/) and try it for your project\. :\-\)