Récemment, ABBYY a publié le code source de son framework NeoML. On nous a proposé de vérifier cette bibliothèque en utilisant PVS-Studio. C'est un projet intéressant du point de vue de l'analyse, donc nous ne l'avons pas remis en veilleuse. Cela ne vous prendra pas beaucoup de temps pour lire cet article, car le projet est de grande qualité :).
Le code source NeoML peut être trouvé sur GitHub . Ce cadre est multiplateforme et est conçu pour implémenter des modèles d'apprentissage automatique. Il est utilisé par les développeurs d'ABBYY pour résoudre les problèmes de vision par ordinateur, le traitement du langage naturel, y compris le traitement d'image, l'analyse de documents, etc. Actuellement, les langages de programmation tels que C ++, Java, Objective-C sont pris en charge et Python sera bientôt ajouté à cette liste. Le langage principal dans lequel le framework lui-même a été écrit est le C ++.
Analyse en cours
Il a été très facile d'exécuter l'analyse sur ce cadre. Après avoir généré un projet Visual Studio dans CMake, j'ai lancé l'analyse PVS-Studio dans Visual Studio pour les projets qui nous intéressent depuis Solution, à l'exclusion des bibliothèques tierces. En plus de NeoML lui-même, la solution comprenait également des bibliothèques d'ABBYY telles que NeoOnnx et NeoMathEngine. Je les ai également inclus dans la liste des projets pour lesquels l'analyse a été lancée.
Les résultats d'analyse
Bien sûr, je voulais vraiment trouver des erreurs terribles, mais ... le code s'est avéré assez propre et les avertissements ont été reçus uniquement, rien. Il est probable que l'analyse statique a déjà été utilisée dans le développement. De nombreux avertissements étaient des déclencheurs des mêmes diagnostics pour des sections similaires du code.
Par exemple, dans ce projet, il est très courant d'appeler une méthode virtuelle à partir d'un constructeur. En général, c'est une approche dangereuse. Le diagnostic V1053 répond à de tels cas : L' appel de la fonction virtuelle 'foo' dans le constructeur / destructeur peut conduire à un résultat inattendu lors de l'exécution . Au total, 10 avertissements de ce type ont été émis. Pour en savoir plus sur les raisons pour lesquelles il s’agit d’une pratique dangereuse et les problèmes qu’elle entraîne, consultez cet article de Scott Myers »Ne jamais appeler des fonctions virtuelles pendant la construction ou la destruction . Cependant, il semble que les développeurs comprennent ce qu'ils font et il n'y a pas d' erreurs.
Il y a aussi 11 V803 diagnostic de niveau moyen des avertissements de la section « microoptimizations ». Ce diagnostic recommande de remplacer l'incrément postfix avec l'incrément de préfixe, si la valeur de l'itérateur précédent n'est pas utilisée. Dans le cas d'un incrément de postfix, un objet temporaire supplémentaire est créé. Bien sûr, ce n'est pas une erreur, juste un petit détail . Si un tel diagnostic n'est pas intéressant, vous pouvez simplement le désactiver lors de l'utilisation de l'analyseur. Eh bien, en principe, le "micro- optimisations "et désactivée par défaut.
En fait, je pense que vous comprenez que depuis que nous sommes arrivés à l'analyse de bagatelles telles que l'incrément de l'itérateur dans l'article, alors tout va généralement bien et nous ne savons tout simplement pas à quoi redire.
Très souvent, certains diagnostics peuvent être inapplicables ou sans intérêt pour l'utilisateur, et il vaut mieux ne pas manger de cactus, mais passer un peu de temps à configurer l'analyseur. Vous pouvez en savoir plus sur les étapes à suivre pour vous rapprocher immédiatement des réponses de l'analyseur les plus intéressantes dans notre article " Comment voir rapidement les avertissements intéressants que l'analyseur PVS-Studio génère pour le code C et C ++? "
Parmi les déclencheurs de la section " les micro-optimisations ont également des avertissements de diagnostic V802 intéressants, qui recommande d'organiser les champs de structure dans l'ordre décroissant des tailles de type, ce qui permet de réduire la taille de la structure.
V802 Sur une plate-forme 64 bits, la taille de la structure peut être réduite de 24 à 16 octets en réorganisant les champs en fonction de leur taille dans l'ordre décroissant. Clustering hiérarchique.h 31
struct CParam {
TDistanceFunc DistanceType;
double MaxClustersDistance;
int MinClustersCount;
};En échangeant simplement le champ MaxClustersDistance avec le type double et l'énumérateur DistanceType , vous pouvez réduire la taille de la structure de 24 à 16 octets.
struct CParam {
double MaxClustersDistance;
int MinClustersCount;
TDistanceFunc DistanceType;
};TDistanceFunc est une énumération , donc sa taille est équivalente à un type int ou plus petit, il doit donc être déplacé à la fin de la structure.
Là encore, ce n'est pas une erreur, mais si l'utilisateur s'intéresse aux microoptimisations ou si elles sont, en principe, importantes pour le projet, de telles opérations d'analyseur peuvent rapidement trouver des endroits pour au moins le refactoring primaire.
En général, tout le code est écrit proprement et lisiblement, mais les diagnostics du V807 ont souligné quelques endroits qui pourraient être rendus un peu plus optimaux et lisibles. Laissez-moi vous donner l'exemple le plus frappant:
V807 Diminution des performances. Envisagez de créer une référence pour éviter d'utiliser la même expression à plusieurs reprises. GradientBoostFullTreeBuilder.cpp 469
L'appel à curLevelStatistics [i] -> ThreadStatistics [j] peut être remplacé par un appel à une variable distincte. Il n'y a pas d'appel de méthodes complexes dans cette chaîne, donc il n'y a peut-être pas d'optimisation spéciale ici, mais le code, à mon avis, sera beaucoup plus facile à lire et aura l'air plus compact. De plus, avec le support de ce code, on verra clairement à l'avenir qu'il est nécessaire d'accéder exactement à ces indices et il n'y a pas d'erreur ici. Pour plus de clarté, je vais donner un code avec un remplacement pour une variable:
auto threadStatistics = curLevelStatistics[i]->ThreadStatistics[j];
if(threadStatistics.FeatureIndex != NotFound ) {
if( threadStatistics.Criterion > criterion
|| ( .... ))
{
criterion = threadStatistics.Criterion;
curLevelStatistics[i]->FeatureIndex = threadStatistics.FeatureIndex;
curLevelStatistics[i]->Threshold = threadStatistics.Threshold;
curLevelStatistics[i]->LeftStatistics = threadStatistics.LeftStatistics;
curLevelStatistics[i]->RightStatistics = threadStatistics.RightStatistics;
}
}Conclusion
Comme vous pouvez le voir, du point de vue de l'analyse statique, la base de code de ce framework s'est avérée très propre.
Il faut comprendre qu'une analyse sur un projet activement développé reflète faiblement la nécessité d'une analyse statique, car de nombreuses erreurs, surtout si elles étaient critiques, ont déjà été détectées par d'autres moyens, mais beaucoup plus chronophages et gourmandes en ressources. Ce point a été analysé plus en détail dans l'article « Erreurs que l'analyse de code statique ne trouve pas car il n'est pas utilisé ».
Mais même avec ce fait à l'esprit, peu d'avertissements ont été émis sur NeoML, et je tiens à exprimer mon respect pour la qualité du code dans ce projet, que les développeurs aient ou non utilisé l'analyse statique.
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: Victoria Khanieva. PVS-Studio impressionné par la qualité du code d'ABBYY NeoML .