And again into space: how the unicorn Stellarium visited
For all the time of its existence, people have made a tremendous amount of effort to study almost the entire area of the starry sky. To date, we have considered hundreds of thousands of asteroids, comets, nebulae and stars, galaxies and planets. To see all this beauty on your own, it is not necessary to leave the house and buy yourself a telescope. You can install on the computer Stellarium - a virtual planetarium, and look at the night sky, lying comfortably on the couch ... But is it comfortable? To find out the answer to this question, check Stellarium for errors in the computer code.
According to the description on Wikipedia, Stellarium is an open source virtual planetarium available for Linux, Mac OS X, Microsoft Windows, Symbian, Android and iOS platforms, and MeeGo. The program uses OpenGL and Qt technologies to create a realistic sky in real time. With Stellarium it is possible to see what can be seen with an average and even large telescope. The program also provides observations of solar eclipses and the movement of comets.
Stellarium was created by French programmer Fabian Chereau, who launched the project in the summer of 2001. Other prominent developers include Robert Spearman, Johannes Hajdozik, Matthew Gates, Timothy Reeves, Bogdan Marinov and Johan Meeris, who are responsible for the artwork.
The project was analyzed using the PVS-Studio static code analyzer. It is a tool for identifying errors and potential vulnerabilities in source code of programs written in C, C ++ and C # languages (in a short time, and in Java!). Works on Windows, Linux and macOS. It is designed for those who need to improve the quality of their code.
To conduct the analysis was simple enough. First, I downloaded the Stellarium project from GitHub, and then installed all the packages needed for building. Since the project is built using Qt Creator, I used a compiler launch tracking system, which is built into the Standalone analyzer version. You can also view the finished analysis report.
New readers and users of Stellariumthey may have wondered: why does the unicorn appear in the title of the article, and how is it related to code analysis? I answer: I am one of the developers of PVS-Studio, and the unicorn is our favorite naughty mascot. So up!
I hope that thanks to this article, readers will learn something new, and the developers of Stellarium will be able to eliminate some of the errors and improve the quality of the code.
Bring yourself a coffee with an air croissant and make yourself comfortable, because we turn to the most interesting - a review of the analysis results and the analysis of errors!
For more pleasure from reading, I suggest not looking directly at the warning of the analyzer, but try here and further to find errors on your own.
PVS-Studio warning : V654 The condition 'start_of_directory == - 1' of loop is always true. qzip.cpp 617 Could you
find a mistake? If yes, then praise.
The error lies in the condition of the while loop . It is always true, since the variable start_of_directory does not change in the body of the loop. Most likely, the cycle will not last forever, since it contains return and break , but this code looks strange.
It seems to me that in the code they forgot to make the start_of_directory = pos assignment in the place where the signature is being checked. Then the break statement is probably superfluous. In this case, the code can be rewritten as:
However, I'm not sure that the code should be exactly like that. It is best that the project developers themselves analyze this part of the program and make the necessary edits.
Another weird condition:
PVS-Studio warning : V501 there ’s the cap.intersects (cap2) and the operator. StelProjectorClasses.hpp 175
As you have probably guessed, the error lies in the last line of the function: the programmer made a typo, and in the end it turned out that the function returns a result regardless of the cap3 value .
This type of error is extremely common: in almost every verified project we encountered typos related to names such as name1 and name2 and the like. As a rule, such errors are related to copy-paste.
This copy of the code is a vivid example of another common error pattern, about which we even conducted a separate mini-study. My colleague Andrei Karpov called it the “ last line effect ”. If you are not familiar with this material, I suggest opening the tab in the browser to read later, but for now we will continue.
PVS-Studio warnings:
The value of the updatePos parameter is always overwritten before it is used, i.e. the function will work the same way, regardless of the value passed to it.
Looks weird, right? In all places where the updatePos parameter is involved , it is true . This means that if (location-> text ()! = NewLocation || updatePos) and if (updatePos) conditions will always be true.
Another snippet:
PVS-Studio warnings:
The analyzer detected a suspicious expression in the arguments of the setFlagAtmosphere and setFlagFog functions . Indeed: on both sides of the bit operator & there are values of type bool . Instead of the & operator , use the && operator , and now I will explain why.
Yes, the result of this expression will always be correct. Before using the bitwise "and" both operands will rise to type int . In C ++, this conversion is straightforward : false is converted to 0, and true is converted to 1. Therefore, the result of this expression will be the same as if the && operator was used .
But there is a nuance. When calculating the result of the && operation , the so-called “lazy evaluation” is used. If the value of the left operand is false , then the right value is not even calculated, because the logical “and” will return false in any case . This is done to save computational resources and allows you to write more complex structures. For example, you can check that the pointer is not null, and, if so, dereference it for additional verification. Example: if (ptr && ptr-> foo ()) .
Such a "lazy evaluation" is not performed when using the bitwise operator & : the expression conf-> value ("...", true) .toBool () will be calculated every timepl-> hasAtmosphere () .
In rare cases, this is done on purpose. For example, if the calculation of the right operand has "side effects", the result of which is used later. It is also not very good to do this, because it complicates the understanding of the code and complicates the care of it. In addition, the order of evaluation of the operands & is not defined, so in some cases of using such "tricks" you can get undefined behavior.
If you want to save side effects - do it on a separate line and save the result in a separate variable. People who will work with this code in the future will be grateful to you :)
Go to the next topic.
Let's start the topic of dynamic memory with this fragment:
PVS-Studio warnings:
The function allocates memory for several structures and passes it to pointers newVertex1 , newVertex2 (interesting names, right?) And newFace . If one of them turns out to be zero, then all the memory reserved within the function is released, after which the control flow leaves the function.
What happens if memory for all three structures is allocated correctly, and the MakeEdge function (& mesh-> eHead) returns NULL ? The control flow reaches the second return .
Since the pointers newVertex1 , newVertex2 and newFaceare local variables, then after exiting the function they will cease to exist. But the release of the memory that belonged to them will not happen. It will remain reserved, but we will no longer have access to it.
Such situations are called "memory leak". A typical scenario with such an error: with a long program running, it begins to consume more and more RAM, until its complete exhaustion.
It should be noted that in this example, the third return is not an error. The MakeVertex and MakeFace functions transfer the addresses of the allocated memory to other data structures, thereby delegating responsibility for its release.
The next flaw is in the method that takes 90 lines. For convenience, I cut it, leaving only problem areas.
There is only one line left. I'll give you a hint: this is the only mention of xs and ys objects .
PVS-Studio warnings:
The xs and ys vectors are created, but not used anywhere. It turns out that every time you use the drawAngularDistanceGraph method, you unnecessarily create and delete an empty container. I think this ad remains in the code after refactoring. This, of course, is not an error, but it is worth removing the extra code.
Another example after a little formatting looks like this:
In order to understand what the error is, you will have to look at the prototypes of the constructors of the Qcolor class :
PVS-Studio warnings:
The Qcolor class does not have constructors that take a double type , so the arguments in the example will be implicitly converted to an int . This causes the buttonColor object's r , g , b fields to be 0 .
If the programmer intended to create an object from double values , he should use another constructor.
For example, you could use a constructor that accepts Qrgb by writing:
It could be done differently. In Qt, real values in the range [0.0, 1.0] or integer in the range [0, 255] are used to designate RGB colors.
Therefore, the programmer could translate values from real to integer by writing this:
or simply
Are you bored? Do not worry: ahead of us are waiting for more interesting mistakes.
"Unicorn in space." View from Stellarium.
Finally, I left you some more tasty things :) Let's get down to one of them.
PVS-Studio warning : V634 The operation of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should not be used in the expression. StelHips.cpp 271
Well, could you detect the error? Consider the expression (12 * 1 << (2 * order)) . The analyzer reminds that the operation ' * ' has a higher priority than the bit-shift operation ' << '. It is easy to understand that multiplying 12 by 1 is meaningless, and brackets around 2 * order are not needed.
Note. Additionally, I want to note that if the value of the right operand ' << ' is greater than or equal to the number of bits of the left operand, the result is undefined. Since numeric literals have int type by default , which takes 32 bits, the value of the order parameter should not exceed 15 . Otherwise, the evaluation of the expression may result in undefined behavior.
We continue. The method given below is quite confusing, but I am sure that the sophisticated reader will cope with the detection of an error :)
PVS-Studio warning : V779 Unreachable code detected. It is possible that an error is present. qcustomplot.cpp 19512.
The fact is that all if ... else branches have a return . Therefore, the control flow never reaches the last two lines.
By and large, this example will run normally and work correctly. But the presence of an unreachable code is itself a signal. In this case, it indicates the wrong structure of the method, which greatly complicates the readability and clarity of the code.
This code fragment should be refactored to produce a tidier function. For example:
The last error in our review will be the one I liked the most. The problem location code is short and simple:
Noticed something suspicious? Not everyone can :)
PVS-Studio Warning: V603 The object was not used. If you wish to call constructor, this-> Plane :: Plane (....) should be used. Plane.cpp 29
The programmer expected that part of the object's fields would be initialized inside the nested constructor, but it turned out that when the Plane constructor is called (Vec3f & v1, Vec3f & v2, Vec3f & v3) , an unnamed temporary object is created inside it, which is immediately deleted. As a result, the part after the object remains uninitialized.
For the code to work properly, you should use a convenient and secure C ++ 11 feature - the delegating constructor:
But if you use the compiler for older versions of the language, you can write this:
Or so:
I note that the last two methods are very dangerous . Therefore, you should be very careful and understand how such methods work.
What conclusions can be made about the quality of the Stellarium code? To be honest, there were not very many mistakes. Also in the whole project I did not find a single error in which the code is tied to indefinite behavior. For an opensource project, the quality of the code was high, for which I take off my hat to the developers. You guys are great! It was pleasant and interesting for me to review your project.
What about the planetarium itself - I use it often enough. Unfortunately, living in the city, I very rarely enjoy the clear night sky, and Stellarium allows me to be anywhere in the world without getting up from the couch. It is really comfortable!
I especially like the “Constellation art” mode. The view of the huge figures covering the whole sky in a strange dance is breathtaking.
"Strange dance." View from Stellarium.
It is natural for us, earthlings, to err, and there is nothing shameful in the fact that these errors leak into the code. For this, code analysis tools such as PVS-Studio are being developed. If you are one of the earthlings - like to offer to download and try it yourself .
I hope that you were interested in reading my article, and you learned something new and useful for yourself. And I wish the developers to fix the errors found as soon as possible.
Subscribe to our channels and follow the news from the world of programming!
If you want to share this article with an English-speaking audience, then please use the link to the translation: George Gribkov. Into Space Again: How the Unicorn Visited Stellarium
A little about the project ...
According to the description on Wikipedia, Stellarium is an open source virtual planetarium available for Linux, Mac OS X, Microsoft Windows, Symbian, Android and iOS platforms, and MeeGo. The program uses OpenGL and Qt technologies to create a realistic sky in real time. With Stellarium it is possible to see what can be seen with an average and even large telescope. The program also provides observations of solar eclipses and the movement of comets.
Stellarium was created by French programmer Fabian Chereau, who launched the project in the summer of 2001. Other prominent developers include Robert Spearman, Johannes Hajdozik, Matthew Gates, Timothy Reeves, Bogdan Marinov and Johan Meeris, who are responsible for the artwork.
... and about the analyzer
The project was analyzed using the PVS-Studio static code analyzer. It is a tool for identifying errors and potential vulnerabilities in source code of programs written in C, C ++ and C # languages (in a short time, and in Java!). Works on Windows, Linux and macOS. It is designed for those who need to improve the quality of their code.
To conduct the analysis was simple enough. First, I downloaded the Stellarium project from GitHub, and then installed all the packages needed for building. Since the project is built using Qt Creator, I used a compiler launch tracking system, which is built into the Standalone analyzer version. You can also view the finished analysis report.
New readers and users of Stellariumthey may have wondered: why does the unicorn appear in the title of the article, and how is it related to code analysis? I answer: I am one of the developers of PVS-Studio, and the unicorn is our favorite naughty mascot. So up!
I hope that thanks to this article, readers will learn something new, and the developers of Stellarium will be able to eliminate some of the errors and improve the quality of the code.
Bring yourself a coffee with an air croissant and make yourself comfortable, because we turn to the most interesting - a review of the analysis results and the analysis of errors!
Suspicious conditions
For more pleasure from reading, I suggest not looking directly at the warning of the analyzer, but try here and further to find errors on your own.
void QZipReaderPrivate::scanFiles()
{
....
// find EndOfDirectory headerint i = 0;
int start_of_directory = -1;
EndOfDirectory eod;
while (start_of_directory == -1) {
constint pos = device->size()
- int(sizeof(EndOfDirectory)) - i;
if (pos < 0 || i > 65535) {
qWarning() << "QZip: EndOfDirectory not found";
return;
}
device->seek(pos);
device->read((char *)&eod, sizeof(EndOfDirectory));
if (readUInt(eod.signature) == 0x06054b50)
break;
++i;
}
....
}PVS-Studio warning : V654 The condition 'start_of_directory == - 1' of loop is always true. qzip.cpp 617 Could you
find a mistake? If yes, then praise.
The error lies in the condition of the while loop . It is always true, since the variable start_of_directory does not change in the body of the loop. Most likely, the cycle will not last forever, since it contains return and break , but this code looks strange.
It seems to me that in the code they forgot to make the start_of_directory = pos assignment in the place where the signature is being checked. Then the break statement is probably superfluous. In this case, the code can be rewritten as:
int i = 0;
int start_of_directory = -1;
EndOfDirectory eod;
while (start_of_directory == -1) {
constint pos = device->size()
- int(sizeof(EndOfDirectory)) - i;
if (pos < 0 || i > 65535) {
qWarning() << "QZip: EndOfDirectory not found";
return;
}
device->seek(pos);
device->read((char *)&eod, sizeof(EndOfDirectory));
if (readUInt(eod.signature) == 0x06054b50)
start_of_directory = pos;
++i;
}However, I'm not sure that the code should be exactly like that. It is best that the project developers themselves analyze this part of the program and make the necessary edits.
Another weird condition:
classStelProjectorCylinder :public StelProjector
{
public:
....
protected:
....
virtualboolintersectViewportDiscontinuityInternal(const Vec3d& capN,
double capD)const{
staticconst SphericalCap cap1(1,0,0);
staticconst SphericalCap cap2(-1,0,0);
staticconst SphericalCap cap3(0,0,-1);
SphericalCap cap(capN, capD);
return cap.intersects(cap1)
&& cap.intersects(cap2)
&& cap.intersects(cap2);
}
};PVS-Studio warning : V501 there ’s the cap.intersects (cap2) and the operator. StelProjectorClasses.hpp 175
As you have probably guessed, the error lies in the last line of the function: the programmer made a typo, and in the end it turned out that the function returns a result regardless of the cap3 value .
This type of error is extremely common: in almost every verified project we encountered typos related to names such as name1 and name2 and the like. As a rule, such errors are related to copy-paste.
This copy of the code is a vivid example of another common error pattern, about which we even conducted a separate mini-study. My colleague Andrei Karpov called it the “ last line effect ”. If you are not familiar with this material, I suggest opening the tab in the browser to read later, but for now we will continue.
void BottomStelBar::updateText(bool updatePos)
{
....
updatePos = true;
....
if (location->text() != newLocation || updatePos)
{
updatePos = true;
....
}
....
if (fov->text() != str)
{
updatePos = true;
....
}
....
if (fps->text() != str)
{
updatePos = true;
....
}
if (updatePos)
{
....
}
}PVS-Studio warnings:
- V560 A part of the conditional expression is always true: updatePos. StelGuiItems.cpp 732
- V547 Expression 'updatePos' is always true. StelGuiItems.cpp 831
- V763 the Parameter 'updatePos' is always rewritten in old body function being of the before Used. StelGuiItems.cpp 690
The value of the updatePos parameter is always overwritten before it is used, i.e. the function will work the same way, regardless of the value passed to it.
Looks weird, right? In all places where the updatePos parameter is involved , it is true . This means that if (location-> text ()! = NewLocation || updatePos) and if (updatePos) conditions will always be true.
Another snippet:
void LandscapeMgr::onTargetLocationChanged(StelLocation loc)
{
....
if (pl && flagEnvironmentAutoEnabling)
{
QSettings* conf = StelApp::getInstance().getSettings();
setFlagAtmosphere(pl->hasAtmosphere()
& conf->value("landscape/flag_atmosphere", true).toBool());
setFlagFog(pl->hasAtmosphere()
& conf->value("landscape/flag_fog", true).toBool());
setFlagLandscape(true);
}
....
}PVS-Studio warnings:
- V792 The 'toBool' function of the operand. Perhaps, it is better to use '&&'. LandscapeMgr.cpp 782
- V792 The 'toBool' function of the operand. Perhaps, it is better to use '&&'. LandscapeMgr.cpp 783
The analyzer detected a suspicious expression in the arguments of the setFlagAtmosphere and setFlagFog functions . Indeed: on both sides of the bit operator & there are values of type bool . Instead of the & operator , use the && operator , and now I will explain why.
Yes, the result of this expression will always be correct. Before using the bitwise "and" both operands will rise to type int . In C ++, this conversion is straightforward : false is converted to 0, and true is converted to 1. Therefore, the result of this expression will be the same as if the && operator was used .
But there is a nuance. When calculating the result of the && operation , the so-called “lazy evaluation” is used. If the value of the left operand is false , then the right value is not even calculated, because the logical “and” will return false in any case . This is done to save computational resources and allows you to write more complex structures. For example, you can check that the pointer is not null, and, if so, dereference it for additional verification. Example: if (ptr && ptr-> foo ()) .
Such a "lazy evaluation" is not performed when using the bitwise operator & : the expression conf-> value ("...", true) .toBool () will be calculated every timepl-> hasAtmosphere () .
In rare cases, this is done on purpose. For example, if the calculation of the right operand has "side effects", the result of which is used later. It is also not very good to do this, because it complicates the understanding of the code and complicates the care of it. In addition, the order of evaluation of the operands & is not defined, so in some cases of using such "tricks" you can get undefined behavior.
If you want to save side effects - do it on a separate line and save the result in a separate variable. People who will work with this code in the future will be grateful to you :)
Go to the next topic.
Wrong work with memory
Let's start the topic of dynamic memory with this fragment:
/************ Basic Edge Operations ****************//* __gl_meshMakeEdge creates one edge,
* two vertices, and a loop (face).
* The loop consists of the two new half-edges.
*/
GLUEShalfEdge* __gl_meshMakeEdge(GLUESmesh* mesh)
{
GLUESvertex* newVertex1 = allocVertex();
GLUESvertex* newVertex2 = allocVertex();
GLUESface* newFace = allocFace();
GLUEShalfEdge* e;
/* if any one is null then all get freed */if ( newVertex1 == NULL
|| newVertex2 == NULL
|| newFace == NULL)
{
if (newVertex1 != NULL)
{
memFree(newVertex1);
}
if (newVertex2 != NULL)
{
memFree(newVertex2);
}
if (newFace != NULL)
{
memFree(newFace);
}
returnNULL;
}
e = MakeEdge(&mesh->eHead);
if (e == NULL)
{
returnNULL;
}
MakeVertex(newVertex1, e, &mesh->vHead);
MakeVertex(newVertex2, e->Sym, &mesh->vHead);
MakeFace(newFace, e, &mesh->fHead);
return e;
}PVS-Studio warnings:
- V773 of The WAS function exited without releasing the 'newVertex1' pointer. A memory leak is possible. mesh.c 312
- V773 The function of the newVertex2 pointer. A memory leak is possible. mesh.c 312
- V773 The function of the newFace pointer. A memory leak is possible. mesh.c 312
The function allocates memory for several structures and passes it to pointers newVertex1 , newVertex2 (interesting names, right?) And newFace . If one of them turns out to be zero, then all the memory reserved within the function is released, after which the control flow leaves the function.
What happens if memory for all three structures is allocated correctly, and the MakeEdge function (& mesh-> eHead) returns NULL ? The control flow reaches the second return .
Since the pointers newVertex1 , newVertex2 and newFaceare local variables, then after exiting the function they will cease to exist. But the release of the memory that belonged to them will not happen. It will remain reserved, but we will no longer have access to it.
Such situations are called "memory leak". A typical scenario with such an error: with a long program running, it begins to consume more and more RAM, until its complete exhaustion.
It should be noted that in this example, the third return is not an error. The MakeVertex and MakeFace functions transfer the addresses of the allocated memory to other data structures, thereby delegating responsibility for its release.
The next flaw is in the method that takes 90 lines. For convenience, I cut it, leaving only problem areas.
void AstroCalcDialog::drawAngularDistanceGraph()
{
....
QVector<double> xs, ys;
....
}There is only one line left. I'll give you a hint: this is the only mention of xs and ys objects .
PVS-Studio warnings:
- V808 'xs' object of 'QVector' type created but not used. AstroCalcDialog.cpp 5329
- V808 'ys' object of 'QVector' type created but not used. AstroCalcDialog.cpp 5329
The xs and ys vectors are created, but not used anywhere. It turns out that every time you use the drawAngularDistanceGraph method, you unnecessarily create and delete an empty container. I think this ad remains in the code after refactoring. This, of course, is not an error, but it is worth removing the extra code.
Strange type conversions
Another example after a little formatting looks like this:
void SatellitesDialog::updateSatelliteData()
{
....
// set default
buttonColor = QColor(0.4, 0.4, 0.4);
....
}In order to understand what the error is, you will have to look at the prototypes of the constructors of the Qcolor class :
PVS-Studio warnings:
- V674 The literal '0.4' of 'double' type is being implicitly casting the 'int' type while calling the 'QColor' function. Inspect the first argument. SatellitesDialog.cpp 413
- V674 The literal '0.4' of 'double' type is being implicitly casting the 'int' type while calling the 'QColor' function. Inspect the second argument. SatellitesDialog.cpp 413
- V674 The literal '0.4' of 'double' type is being implicitly casting the 'int' type while calling the 'QColor' function. Inspect the third argument. SatellitesDialog.cpp 413
The Qcolor class does not have constructors that take a double type , so the arguments in the example will be implicitly converted to an int . This causes the buttonColor object's r , g , b fields to be 0 .
If the programmer intended to create an object from double values , he should use another constructor.
For example, you could use a constructor that accepts Qrgb by writing:
buttonColor = QColor(QColor::fromRgbF(0.4, 0.4, 0.4));It could be done differently. In Qt, real values in the range [0.0, 1.0] or integer in the range [0, 255] are used to designate RGB colors.
Therefore, the programmer could translate values from real to integer by writing this:
buttonColor = QColor((int)(255 * 0.4),
(int)(255 * 0.4),
(int)(255 * 0.4));or simply
buttonColor = QColor(102, 102, 102);Are you bored? Do not worry: ahead of us are waiting for more interesting mistakes.
"Unicorn in space." View from Stellarium.
Other errors
Finally, I left you some more tasty things :) Let's get down to one of them.
HipsTile* HipsSurvey::getTile(int order, int pix)
{
....
if (order == orderMin && !allsky.isNull())
{
int nbw = sqrt(12 * 1 << (2 * order));
int x = (pix % nbw) * allsky.width() / nbw;
int y = (pix / nbw) * allsky.width() / nbw;
int s = allsky.width() / nbw;
QImage image = allsky.copy(x, y, s, s);
....
}
....
}PVS-Studio warning : V634 The operation of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should not be used in the expression. StelHips.cpp 271
Well, could you detect the error? Consider the expression (12 * 1 << (2 * order)) . The analyzer reminds that the operation ' * ' has a higher priority than the bit-shift operation ' << '. It is easy to understand that multiplying 12 by 1 is meaningless, and brackets around 2 * order are not needed.
Скорее всего, программист собирался написать так:
int nbw = sqrt(12 * (1 << 2 * order));
В таком случае значение <i>12 </i> будет умножаться на корректное число.Note. Additionally, I want to note that if the value of the right operand ' << ' is greater than or equal to the number of bits of the left operand, the result is undefined. Since numeric literals have int type by default , which takes 32 bits, the value of the order parameter should not exceed 15 . Otherwise, the evaluation of the expression may result in undefined behavior.
We continue. The method given below is quite confusing, but I am sure that the sophisticated reader will cope with the detection of an error :)
/* inherits documentation from base class */
QCPRange QCPStatisticalBox::
getKeyRange(bool& foundRange, SignDomain inSignDomain) const
{
foundRange = true;
if (inSignDomain == sdBoth)
{
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
}
elseif (inSignDomain == sdNegative)
{
if (mKey + mWidth * 0.5 < 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
elseif (mKey < 0)
return QCPRange(mKey - mWidth * 0.5, mKey);
else
{
foundRange = false;
return QCPRange();
}
}
elseif (inSignDomain == sdPositive)
{
if (mKey - mWidth * 0.5 > 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
elseif (mKey > 0)
return QCPRange(mKey, mKey + mWidth * 0.5);
else
{
foundRange = false;
return QCPRange();
}
}
foundRange = false;
return QCPRange();
}PVS-Studio warning : V779 Unreachable code detected. It is possible that an error is present. qcustomplot.cpp 19512.
The fact is that all if ... else branches have a return . Therefore, the control flow never reaches the last two lines.
By and large, this example will run normally and work correctly. But the presence of an unreachable code is itself a signal. In this case, it indicates the wrong structure of the method, which greatly complicates the readability and clarity of the code.
This code fragment should be refactored to produce a tidier function. For example:
/* inherits documentation from base class */
QCPRange QCPStatisticalBox::
getKeyRange(bool& foundRange, SignDomain inSignDomain) const
{
foundRange = true;
switch (inSignDomain)
{
case sdBoth:
{
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
break;
}
case sdNegative:
{
if (mKey + mWidth * 0.5 < 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
elseif (mKey < 0)
return QCPRange(mKey - mWidth * 0.5, mKey);
break;
}
case sdPositive: {
if (mKey - mWidth * 0.5 > 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
elseif (mKey > 0)
return QCPRange(mKey, mKey + mWidth * 0.5);
break;
}
}
foundRange = false;
return QCPRange();
}The last error in our review will be the one I liked the most. The problem location code is short and simple:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3)
: distance(0.0f), sDistance(0.0f)
{
Plane(v1, v2, v3, SPolygon::CCW);
}Noticed something suspicious? Not everyone can :)
PVS-Studio Warning: V603 The object was not used. If you wish to call constructor, this-> Plane :: Plane (....) should be used. Plane.cpp 29
The programmer expected that part of the object's fields would be initialized inside the nested constructor, but it turned out that when the Plane constructor is called (Vec3f & v1, Vec3f & v2, Vec3f & v3) , an unnamed temporary object is created inside it, which is immediately deleted. As a result, the part after the object remains uninitialized.
For the code to work properly, you should use a convenient and secure C ++ 11 feature - the delegating constructor:
Plane::Plane(Vec3f& v1, Vec3f& v2, Vec3f& v3)
: Plane(v1, v2, v3, SPolygon::CCW)
{
distance = 0.0f;
sDistance = 0.0f;
}But if you use the compiler for older versions of the language, you can write this:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3)
: distance(0.0f), sDistance(0.0f)
{
this->Plane::Plane(v1, v2, v3, SPolygon::CCW);
}Or so:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3)
: distance(0.0f), sDistance(0.0f)
{
new (this) Plane(v1, v2, v3, SPolygon::CCW);
}I note that the last two methods are very dangerous . Therefore, you should be very careful and understand how such methods work.
Conclusion
What conclusions can be made about the quality of the Stellarium code? To be honest, there were not very many mistakes. Also in the whole project I did not find a single error in which the code is tied to indefinite behavior. For an opensource project, the quality of the code was high, for which I take off my hat to the developers. You guys are great! It was pleasant and interesting for me to review your project.
What about the planetarium itself - I use it often enough. Unfortunately, living in the city, I very rarely enjoy the clear night sky, and Stellarium allows me to be anywhere in the world without getting up from the couch. It is really comfortable!
I especially like the “Constellation art” mode. The view of the huge figures covering the whole sky in a strange dance is breathtaking.
"Strange dance." View from Stellarium.
It is natural for us, earthlings, to err, and there is nothing shameful in the fact that these errors leak into the code. For this, code analysis tools such as PVS-Studio are being developed. If you are one of the earthlings -
I hope that you were interested in reading my article, and you learned something new and useful for yourself. And I wish the developers to fix the errors found as soon as possible.
Subscribe to our channels and follow the news from the world of programming!
- Vk: @pvsstudio_rus
- Telegram: @pvsstudio_rus
- Instagram: @pvsstudio_rus
- Twitter: @pvsstudio_rus
- YouTube: @PVSStudioTool
If you want to share this article with an English-speaking audience, then please use the link to the translation: George Gribkov. Into Space Again: How the Unicorn Visited Stellarium