Notre attention a récemment été attirée par le référentiel Electronic Arts sur GitHub. Il est minuscule et sur les vingt-trois projets disponibles, seules quelques bibliothèques C ++ semblaient intéressantes: EASTL, EAStdC, EABase, EAThread, EATest, EAMain et EAAssert. Les projets eux-mêmes sont également minuscules (environ 10 fichiers chacun), donc des bogues n'ont été trouvés que dans le "plus grand" projet de 20 fichiers: D Mais nous les avons trouvés et ils ont l'air intéressants! Au moment où j'écrivais cet article, nous avons également eu une discussion animée sur les jeux EA et la politique de l'entreprise: D
Présentation
Electronic Arts (EA) est une société américaine de jeux vidéo. Il possède un petit
référentiel sur GitHub et quelques projets C ++, à savoir les bibliothèques C ++: EASTL, EAStdC, EABase, EAThread, EATest, EAMain et EAAssert. Ils sont minuscules et l'analyseur
PVS-Studio n'a réussi à trouver des bogues que dans le "plus grand" projet,
EAStdC (20 fichiers). Avec de telles tailles, vous
ne pouvez pas juger de
manière fiable la qualité globale du code, alors jetez un coup d'œil aux cinq avertissements suivants et décidez par vous-même.
Avertissement 1
V524 Il est étrange que le corps de la fonction '>>' soit entièrement équivalent au corps de la fonction '<<'. 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;} .... }
Lors de la surcharge des opérateurs de décalage, le programmeur a fait une faute de frappe dans l'un d'eux en écrivant << au lieu de >>. Cela ressemble beaucoup à une erreur de copier-coller.
Avertissement 2
V557 Le dépassement de matrice est possible. La valeur de l'index 'nFormatLength' pourrait atteindre 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;
Le tableau
spans [spanIndex] .mFormat se compose de
16 éléments, donc le dernier index d'élément valide est
15 . Dans sa forme actuelle, la fonction
OVprintfCore finit par incrémenter l'index de
nFormatLength à
16 si elle a l'indice le plus élevé possible, c'est-à-dire
15 . Après cela, une erreur de tableau hors limites se produira dans l'instruction
switch .
Ce fragment a été copié deux fois de plus:
- V557 Le dépassement de matrice est possible. La valeur de l'index 'nFormatLength' pourrait atteindre 16. EASprintfOrdered.cpp 614
- V557 Le dépassement de matrice est possible. La valeur de l'index 'nFormatLength' pourrait atteindre 16. EASprintfOrdered.cpp 977
Avertissement 3
V560 Une partie de l'expression conditionnelle est toujours vraie: (résultat> = 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; } .... }
La condition
résultat> = 0 est toujours vraie car la variable de
résultat n'est modifiée nulle part dans la boucle. Le code ne semble pas du tout correct, et il doit y avoir une erreur.
Ce fragment a été copié deux fois de plus:
- V560 Une partie de l'expression conditionnelle est toujours vraie: (résultat> = 0). EASprintfOrdered.cpp 852
- V560 Une partie de l'expression conditionnelle est toujours vraie: (résultat> = 0). EASprintfOrdered.cpp 1215
Avertissement 4
V1009 Vérifiez l'initialisation de la baie. Seul le premier élément est initialisé explicitement. Les éléments restants sont initialisés avec des zéros. EASprintfOrdered.cpp 151
static int OVprintfCore(....) { .... int spanArgOrder[kArgCapacity] = { -1 }; .... }
Ce n'est pas nécessairement un bogue, mais les auteurs doivent être avertis que seul le premier élément du tableau
spanArgOrder est initialisé à
-1 , tandis que tout le reste sera mis à 0.
Ce fragment a été copié deux fois de plus:
- V1009 Vérifiez l'initialisation de la baie. Seul le premier élément est initialisé explicitement. Les éléments restants sont initialisés avec des zéros. EASprintfOrdered.cpp 518
- V1009 Vérifiez l'initialisation de la baie. Seul le premier élément est initialisé explicitement. Les éléments restants sont initialisés avec des zéros. EASprintfOrdered.cpp 881
Avertissement 5
V728 Un contrôle excessif peut être simplifié. Le '(A &&! B) || (! A && B) 'est équivalente à l'expression' bool (A)! = Bool (B) '. int128.h 1242
inline void int128_t::Modulus(....) const { .... bool bDividendNegative = false; bool bDivisorNegative = false; .... if( (bDividendNegative && !bDivisorNegative) || (!bDividendNegative && bDivisorNegative)) { quotient.Negate(); } .... }
J'ai formaté cet exemple pour plus de clarté, mais dans sa forme originale, cette condition est très longue et difficile à lire. Mais nous pouvons faire beaucoup mieux en simplifiant l'expression conditionnelle, comme le suggère l'analyseur:
if( bDividendNegative != bDivisorNegative) { quotient.Negate(); }
Le code est beaucoup plus court maintenant, ce qui rend la logique de la condition beaucoup plus facile à saisir.
Conclusion
Comme vous l'avez peut-être remarqué, la plupart des avertissements ont deux doublons supplémentaires, et tous ces doublons se trouvent dans le même fichier. La duplication de code est un très mauvais anti-modèle car elle complique beaucoup la maintenance du programme. Lorsque des bogues se glissent dans un tel code, la stabilité du programme diminue considérablement car ils se propagent partout dans le code.
J'espère que EA téléchargera d'autres projets intéressants et nous visiterons à nouveau leur référentiel :). En attendant, bienvenue pour télécharger
PVS-Studio et l'essayer sur vos propres projets.