Electronic Arts Libraries of Almost Good Quality
Our attention was drawn to the Electronic Arts repository on GitHub. It is very small and out of twenty-three projects we were only interested in a few C ++ libraries: EASTL, EAStdC, EABase, EAThread, EATest, EAMain and EAAssert. Projects were also very small (about 10 files), so we found errors only in the “largest” of 20 files: D But we found interesting ones as well! While the article was being written, my colleagues and I also vigorously discussed EA's games and its strategy: D
Electronic Arts (EA) is an American company that distributes video games. On the GitHub website, she has a small repository and several C ++ projects. These are C ++ libraries: EASTL, EAStdC, EABase, EAThread, EATest, EAMain and EAAssert. They are very small and we found interesting errors using the PVS-Studio analyzer only in the "largest" of them - EAStdC (20 files). With such volumes it is difficult to talk about the quality of the code as a whole. Just rate 5 warnings and decide for yourself.
V524 It is odd that the body of '>>' function is fully equivalent to the body of '<<' function. EAFixedPoint.h 287
')
When overloading shift operators, the programmer made a typo in one of them, confusing the << and >> operators. Most likely, this is the result of Copy-Paste-programming.
V557 Array overrun is possible. The value of 'nFormatLength' index could reach 16. EASprintfOrdered.cpp 246
The spans [spanIndex] .mFormat array consists of 16 elements, i.e. the last valid item has an index of 15 . Now the OVprintfCore function code is written so that if the nFormatLength index has the maximum possible index - 15 , then an increment of up to 16 will occur. Further, in the switch statement, it is possible to go beyond the boundary of the array.
This code fragment was copied to 2 more places:
V560 A part of conditional expression is always true: (result> = 0). EASprintfOrdered.cpp 489
The condition result> = 0 is always true, because the result variable does not change anywhere in the loop. The code looks very suspicious and, most likely, there is an error in this code.
This code fragment was copied to 2 more places:
V1009 Check the array initialization. Only the first element is initialized explicitly. The rest elements are initialized with zeros. EASprintfOrdered.cpp 151
This may not be a mistake, but developers should be warned that only the first element of the spanArgOrder array is initialized with a value of -1 , and all others will have a value of 0.
This code fragment was copied to 2 more places:
V728 An excessive check can be simplified. The '(A &&! B) || (! A && B) 'expression is equivalent to the' bool (A)! = Bool (B) 'expression. int128.h 1242
This code fragment was formatted for convenience, but in the original it is a very long condition that is difficult to read. Another thing is if we simplify the conditional expression, as the analyzer advises:
The code has become much shorter, which greatly simplified the understanding of the logic of conditional expression.
As you may have noticed, most of the suspicious warnings are duplicated in three places, and within the same file. Code duplication is a very bad development practice, as complicates code support. And if an error occurs in such code, then the stability of the program decreases sharply due to the spread of errors throughout the code.
Let's hope that something else interesting will be published and we will return to this repository again :). In the meantime, I suggest those who wish to download PVS-Studio and try to check their own projects.
If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Almost Perfect Libraries by Electronic Arts .
Introduction
Electronic Arts (EA) is an American company that distributes video games. On the GitHub website, she has a small repository and several C ++ projects. These are C ++ libraries: EASTL, EAStdC, EABase, EAThread, EATest, EAMain and EAAssert. They are very small and we found interesting errors using the PVS-Studio analyzer only in the "largest" of them - EAStdC (20 files). With such volumes it is difficult to talk about the quality of the code as a whole. Just rate 5 warnings and decide for yourself.
Warning 1
V524 It is odd that the body of '>>' function is fully equivalent to the body of '<<' function. EAFixedPoint.h 287
')
template <class T, int upShiftInt, int downShiftInt, int upMulInt, int downDivInt> struct FPTemplate { .... FPTemplate operator<<(int numBits) const { return value << numBits; } FPTemplate operator>>(int numBits) const { return value << numBits; } FPTemplate& operator<<=(int numBits) { value <<= numBits; return *this;} FPTemplate& operator>>=(int numBits) { value >>= numBits; return *this;} .... } When overloading shift operators, the programmer made a typo in one of them, confusing the << and >> operators. Most likely, this is the result of Copy-Paste-programming.
Warning 2
V557 Array overrun is possible. The value of 'nFormatLength' index could reach 16. EASprintfOrdered.cpp 246
static const int kSpanFormatCapacity = 16; struct Span8 { .... char mFormat[kSpanFormatCapacity]; .... }; static int OVprintfCore(....) { .... EA_ASSERT(nFormatLength < kSpanFormatCapacity); if(nFormatLength < kSpanFormatCapacity) spans[spanIndex].mFormat[nFormatLength++] = *p; // <= else return -1; switch(*p) { case 'b': case 'd': case 'i': case 'u': case 'o': case 'x': case 'X': case 'g': case 'G': case 'e': case 'E': case 'f': case 'F': case 'a': case 'A': case 'p': case 'c': case 'C': case 's': case 'S': case 'n': { // Finalize the current span. spans[spanIndex].mpEnd = p + 1; spans[spanIndex].mFormat[nFormatLength] = 0; // <= spans[spanIndex].mFormatChar = *p; if(++spanIndex == kSpanCapacity) break; .... } The spans [spanIndex] .mFormat array consists of 16 elements, i.e. the last valid item has an index of 15 . Now the OVprintfCore function code is written so that if the nFormatLength index has the maximum possible index - 15 , then an increment of up to 16 will occur. Further, in the switch statement, it is possible to go beyond the boundary of the array.
This code fragment was copied to 2 more places:
- V557 Array overrun is possible. The value of 'nFormatLength' index could reach 16. EASprintfOrdered.cpp 614
- V557 Array overrun is possible. The value of 'nFormatLength' index could reach 16. EASprintfOrdered.cpp 977
Warning 3
V560 A part of conditional expression is always true: (result> = 0). EASprintfOrdered.cpp 489
static int OVprintfCore(....) { .... for(result = 1; (result >= 0) && (p < pEnd); ++p) { if(pWriteFunction8(p, 1, pWriteFunctionContext8, kWFSIntermediate) < 0) return -1; nWriteCountSum += result; } .... } The condition result> = 0 is always true, because the result variable does not change anywhere in the loop. The code looks very suspicious and, most likely, there is an error in this code.
This code fragment was copied to 2 more places:
- V560 A part of conditional expression is always true: (result> = 0). EASprintfOrdered.cpp 852
- V560 A part of conditional expression is always true: (result> = 0). EASprintfOrdered.cpp 1215
Warning 4
V1009 Check the array initialization. Only the first element is initialized explicitly. The rest elements are initialized with zeros. EASprintfOrdered.cpp 151
static int OVprintfCore(....) { .... int spanArgOrder[kArgCapacity] = { -1 }; .... } This may not be a mistake, but developers should be warned that only the first element of the spanArgOrder array is initialized with a value of -1 , and all others will have a value of 0.
This code fragment was copied to 2 more places:
- V1009 Check the array initialization. Only the first element is initialized explicitly. The rest elements are initialized with zeros. EASprintfOrdered.cpp 518
- V1009 Check the array initialization. Only the first element is initialized explicitly. The rest elements are initialized with zeros. EASprintfOrdered.cpp 881
Warning 5
V728 An excessive check can be simplified. The '(A &&! B) || (! A && B) 'expression is equivalent to the' bool (A)! = Bool (B) 'expression. int128.h 1242
inline void int128_t::Modulus(....) const { .... bool bDividendNegative = false; bool bDivisorNegative = false; .... if( (bDividendNegative && !bDivisorNegative) || (!bDividendNegative && bDivisorNegative)) { quotient.Negate(); } .... } This code fragment was formatted for convenience, but in the original it is a very long condition that is difficult to read. Another thing is if we simplify the conditional expression, as the analyzer advises:
if( bDividendNegative != bDivisorNegative) { quotient.Negate(); } The code has become much shorter, which greatly simplified the understanding of the logic of conditional expression.
Conclusion
As you may have noticed, most of the suspicious warnings are duplicated in three places, and within the same file. Code duplication is a very bad development practice, as complicates code support. And if an error occurs in such code, then the stability of the program decreases sharply due to the spread of errors throughout the code.
Let's hope that something else interesting will be published and we will return to this repository again :). In the meantime, I suggest those who wish to download PVS-Studio and try to check their own projects.
If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Almost Perfect Libraries by Electronic Arts .
Source: https://habr.com/ru/post/461679/
All Articles