The majority of the projects we report about in these articles contain dozens of PVS-Studio analyzer warnings. Of course we choose just a small portion of data from the analyzer report to be in our articles. There are some projects though, where the quantity of warnings is not that high and the number of some interesting "bloomers" is just not enough for an article. Usually these are small projects, which ceased developing. Today I'm going to tell you about Appleseed project check, the code of which we found quite high-quality, from the point of view of the analyzer.
Introduction
Appleseed is a modern, open source, physically-based rendering engine designed to produce photorealistic images, animations and visual effects. It provides individuals and small studios with an efficient, reliable suite of tools built on robust foundations and open technologies.
This project contains 700 source code files. Our
PVS-Studio analyzer found just several warnings of 1st and 2nd level that could be of interest to us.
Check Results
V670 The uninitialized class member m_s0_cache is used to initialize the m_s1_element_swapper member. Remember that members are initialized in the order of their declarations inside a class. animatecamera cache.h 1009
class DualStageCache
: public NonCopyable
{
....
S1ElementSwapper m_s1_element_swapper; //<==Line 679
S1Cache m_s1_cache;
S0ElementSwapper m_s0_element_swapper;
S0Cache m_s0_cache; //<==Line 683
};
FOUNDATION_DSCACHE_TEMPLATE_DEF(APPLESEED_EMPTY)
DualStageCache(
KeyHasherType& key_hasher,
ElementSwapperType& element_swapper,
const KeyType& invalid_key,
AllocatorType allocator)
: m_s1_element_swapper(m_s0_cache, element_swapper)//warning...
// warning: referring to an uninitialized member
, m_s1_cache(m_s1_element_swapper, allocator)
, m_s0_element_swapper(m_s1_cache)
, m_s0_cache(key_hasher, m_s0_element_swapper, invalid_key)
{
}
The analyzer found a possible error in the constructor class initialization. Judging by the comment: "warning: referring to an uninitialized member", which has already been in the code, we see that the developers know that for the
m_s1_element_swapper field initialization another uninitialized
m_s0_cache field may be used. They are not correcting it though. According to the language standard, the order of initialization of the class members in the constructor goes in their declaration order in the class.
V605 Consider verifying the expression: m_variation_aov_index [lessthan] ~0. An unsigned value is compared to the number -1. appleseed adaptivepixelrenderer.cpp 154
size_t m_variation_aov_index;
size_t m_samples_aov_index;
virtual void on_tile_end(
const Frame& frame,
Tile& tile,
TileStack& aov_tiles) APPLESEED_OVERRIDE
{
....
if (m_variation_aov_index < ~0) //<==
aov_tiles.set_pixel(x, y, m_variation_aov_index, ....);
if (m_samples_aov_index != ~0) //<==
aov_tiles.set_pixel(x, y, m_samples_aov_index, ....);
....
}
The inversion result of
~0 is -1, having the
int type. Then this number converts into an unsigned
size_t type. It's not crucial, but not really graceful. It is recommended to specify a
SIZE_MAX constant in such expression right away.
At first glance there is no evident error here. But my attention was drawn by the usage of two different conditional operators, though both conditions check the same. The conditions are true if the variables are not equal to the maximum possible
size_t type value (
SIZE_MAX). These checks are differently written. Such a code looks very suspicious; perhaps there can be some logical error here.
V668 There is no sense in testing the 'result' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. appleseed string.cpp 58
char* duplicate_string(const char* s)
{
assert(s);
char* result = new char[strlen(s) + 1];
if (result)
strcpy(result, s);
return result;
}
The analyzer detected a situation when the pointer value, returned by the
new operator, is compared to null. We should remember, that if the
new operator could not allocate the memory, then according to the C++ language standard, an exception
std::bad_alloc() would be generated.
Thus in the Appleseed project, which is compiled into Visual Studio 2013, the pointer comparison with null will be meaningless. One day such function usage can lead to an unexpected result. It is assumed that the
duplicate_string() function will return
nullptr if it can't create a string duplicate. It will generate an exception instead, that other parts of the program may be not ready for.
V719 The switch statement does not cover all values of the 'InputFormat' enum: InputFormatEntity. appleseed inputarray.cpp 92
enum InputFormat
{
InputFormatScalar,
InputFormatSpectralReflectance,
InputFormatSpectralIlluminance,
InputFormatSpectralReflectanceWithAlpha,
InputFormatSpectralIlluminanceWithAlpha,
InputFormatEntity
};
size_t add_size(size_t size) const
{
switch (m_format)
{
case InputFormatScalar:
....
case InputFormatSpectralReflectance:
case InputFormatSpectralIlluminance:
....
case InputFormatSpectralReflectanceWithAlpha:
case InputFormatSpectralIlluminanceWithAlpha:
....
}
return size;
}
And where is the case for
InputFormatEntity? This
switch() block contains neither a default section, nor a variable action with the
InputFormatEntity value. Is it a real error or did the author deliberately miss the value?
There are two more fragments (cases) like that:
- V719 The switch statement does not cover all values of the InputFormat enum: InputFormatEntity. appleseed inputarray.cpp 121
- V719 The switch statement does not cover all values of the InputFormat enum: InputFormatEntity. appleseed inputarray.cpp 182
If there is no
default section and handling of all variable values, you may possibly miss the code addition for a new
InputFormat value and not be aware of that for a very long time.
V205 Explicit conversion of pointer type to 32-bit integer type: (unsigned long int) strvalue appleseed snprintf.cpp 885
#define UINTPTR_T unsigned long int
int
portable_vsnprintf(char *str, size_t size, const char *format,
va_list args)
{
const char *strvalue;
....
fmtint(str, &len, size,
(UINTPTR_T)strvalue, 16, width, //<==
precision, flags);
....
}
Finally we found quite a serious error that shows up in a 64-bit version of the program. Appleseed is a cross-platform project that can be compiled on Windows and Linux. To get the project files we use Cmake. In the Windows compilation documentation it is suggested to use "Visual Studio 12 Win64" that's why except the general diagnostics (GA, General Analysis), I've also looked through the diagnostics of 64-bit errors (64, Viva64) of the PVS-Studio analyzer.
The full identification code of
UINTPTR_T macro looks like this:
/* Support for uintptr_t. */
#ifndef UINTPTR_T
#if HAVE_UINTPTR_T || defined(uintptr_t)
#define UINTPTR_T uintptr_t
#else
#define UINTPTR_T unsigned long int
#endif /* HAVE_UINTPTR_T || defined(uintptr_t) */
#endif /* !defined(UINTPTR_T) */
The
uintptr_t is an unsigned, integer memsize-type that can safely hold a pointer no matter what the platform architecture is, although for Windows compilation was defined
unsigned long int type. The type size depends on the
data model, and unlike Linux OS, the
long type is always 32-bits in Windows. That's why the pointer won't fit into this variable type on Win64 platform.
Conclusion
All in all the Appleseed project, which is quite a big one, contains only a few analyzer's warnings. That's why it proudly gets a medal "Clear Code" and can no longer be afraid of our unicorn.
Concerning banners and advertisement, we definitely try these channels as well, but the effect is much less than from an article publication. If you have any other questions, feel free to ask.
Guys, this is what sending a pull request is for, not for a public airing of open-sourced dirty laundry. If all you want to do is market your code analysis tools, buy a banner.
(I'm calling this comment a positive, constructive one, because it gives specific advice that would improve PVS-Studio's public image and reputation in the eyes of the people likely to use it)