Il est mauvais de combiner plusieurs actions dans une seule expression du langage C ++, car un tel code est difficile à comprendre, difficile à maintenir, et il est également facile de s'y tromper. Par exemple, créez un bogue en combinant différentes actions lors de l'évaluation des arguments de fonction. Nous sommes d'accord avec la recommandation classique selon laquelle le code doit être simple et direct. Et maintenant, considérons un cas intéressant, lorsque l'analyseur PVS-Studio est formellement erroné, mais d'un point de vue pratique, le code doit encore être modifié.
Ordre d'évaluation des arguments
Ce qui sera raconté maintenant est une continuation de la vieille histoire sur l'ordre de calcul des arguments, dont nous avons parlé dans l'article " La profondeur d'un terrier de lapin ou une interview en C ++ chez PVS-Studio ".
La brève essence est la suivante. L'ordre dans lequel les arguments de fonction sont évalués est un comportement non spécifié. La norme ne spécifie pas l'ordre exact dans lequel les développeurs du compilateur doivent évaluer les arguments. Par exemple, de gauche à droite (Clang) ou de droite à gauche (GCC, MSVC). Avant la norme C ++ 17, lorsque des effets secondaires se produisaient lors de l'évaluation des arguments, cela pouvait conduire à un comportement indéfini.
Avec l'avènement du standard C ++ 17, la situation a changé pour le mieux: désormais, le calcul de l'argument et ses effets secondaires ne commenceront à être effectués qu'à partir du moment où tous les calculs et effets secondaires de l'argument précédent seront effectués. Cependant, cela ne signifie pas qu'il n'y a plus de place pour l'erreur.
Considérez un programme de test simple:
#include <cstdio>
int main()
{
int i = 1;
printf("%d, %d\n", i, i++);
return 0;
}
Qu'est-ce que ce code imprimera? La réponse dépend toujours du compilateur, de la version et de l'humeur. Selon le compilateur, «1, 1» et «2, 1» peuvent être imprimés. En effet, en utilisant l'Explorateur de compilateurs, j'obtiens les résultats suivants:
- un programme compilé avec Clang 11.0.0 produit "1, 1".
- un programme compilé avec GCC 10.2 produit "2, 1".
Il n'y a pas de comportement indéfini dans ce programme, mais il y a un comportement non spécifié (ordre d'évaluation des arguments).
Code du projet CSV Parser
Revenons à l'extrait de code du projet CSV Parser, que j'ai mentionné dans l'article " Vérification de la collection de bibliothèques C ++ en-tête uniquement (awesome-hpp) ".
L'analyseur et moi savons que les arguments peuvent être évalués dans un ordre différent. Par conséquent, l'analyseur, et après lui-même, a considéré ce code comme erroné:
std::unique_ptr<char[]> buffer(new char[BUFFER_UPPER_LIMIT]);
....
this->feed_state->feed_buffer.push_back(
std::make_pair<>(std::move(buffer), line_buffer - buffer.get()));
Avertissement PVS-Studio: V769 Le pointeur 'buffer.get ()' dans l'expression 'line_buffer - buffer.get ()' est égal à nullptr. La valeur résultante est insensée et ne doit pas être utilisée. csv.hpp 4957
En fait, nous avons tous les deux tort et il n'y a pas d'erreur. Nous parlerons plus en détail des nuances, mais pour l'instant, commençons par une simple.
Alors, voyons pourquoi il est dangereux d'écrire du code comme celui-ci:
Foo(std::move(buffer), line_buffer - buffer.get());
Je pense que vous pouvez deviner la réponse. Le résultat dépend de l'ordre dans lequel les arguments sont évalués. Considérez ceci dans le code synthétique suivant:
#include <iostream>
#include <memory>
void Print(std::unique_ptr<char[]> p, ptrdiff_t diff)
{
std::cout << diff << std::endl;
}
void Print2(ptrdiff_t diff, std::unique_ptr<char[]> p)
{
std::cout << diff << std::endl;
}
int main()
{
{
std::unique_ptr<char[]> buffer(new char[100]);
char *ptr = buffer.get() + 22;
Print(std::move(buffer), ptr - buffer.get());
}
{
std::unique_ptr<char[]> buffer(new char[100]);
char *ptr = buffer.get() + 22;
Print2(ptr - buffer.get(), std::move(buffer));
}
return 0;
}
Utilisons à nouveau l'explorateur de compilateurs et voyons la sortie de ce programme compilé par différents compilateurs.
Compilateur Clang 11.0.0. Résultat :
23387846
22
Le compilateur GCC 10.2. Résultat :
22
26640070
Nous attendons le résultat, et vous ne pouvez pas écrire comme ça. C'est exactement ce sur quoi l'analyseur PVS-Studio met en garde.
Je voudrais mettre un terme à cela, mais tout est un peu plus compliqué. Le fait est que nous parlons de passer des arguments par valeur, et lors de l'instanciation du modèle de fonction std :: make_pair, tout sera différent. Continuons à plonger dans les nuances et découvrons pourquoi PVS-Studio se trompe dans ce cas.
std :: make_pair
Jetons un coup d'œil au site Web de cppreference et voyons comment le modèle de fonction std :: make_pair a changé .
Jusqu'au C ++ 11:
template <classe T1, classe T2>Depuis C ++ 11, jusqu'à C ++ 14:
std :: pair <T1, T2> make_pair (T1 t, T2 u);
template <classe T1, classe T2>Depuis C ++ 14:
std :: pair <V1, V2> make_pair (T1 && t, T2 && u);
template <classe T1, classe T2>Comme vous pouvez le voir, il était une fois std :: make_pair prenait des arguments par valeur. Si std :: unique_ptr existait à ce moment-là , alors le code ci-dessus serait en effet incorrect. Que ce code fonctionne ou non était une question de chance. En pratique, bien sûr, cette situation ne se serait jamais produite, puisque std :: unique_ptr est apparu dans C ++ 11 en remplacement de std :: auto_ptr .
constexpr std :: pair <V1, V2> make_pair (T1 && t, T2 && u);
Revenons à notre époque. À partir de la version du standard C ++ 11, le constructeur a commencé à utiliser la sémantique de déplacement.
Il y a un point subtil ici que std :: movene déplace réellement rien, mais convertit simplement l'objet en référence rvalue . Cela permettra à std :: make_pair de passer le pointeur vers le nouveau std :: unique_ptr , laissant nullptr dans le pointeur intelligent d'origine. Mais ce passage de pointeur ne se produira pas tant que nous n'entrons pas dans std :: make_pair . À ce moment-là, nous avons déjà calculé line_buffer - buffer.get () , et tout ira bien. Autrement dit, un appel à buffer.get () ne peut pas retourner nullptrau moment où il est calculé, peu importe quand exactement cela se produit.
Je suis désolé pour la description compliquée. L'essentiel est que ce code est parfaitement valide. Et en fait, l'analyseur statique PVS-Studio dans ce cas a donné une fausse alarme. Cependant, notre équipe n'est pas sûre que nous devions nous précipiter pour apporter des modifications à la logique de l'analyseur pour de telles situations.
Le roi est mort, longue vie au roi!
Nous avons découvert que l'opération décrite dans l' article s'est avérée fausse. Merci à l'un de nos lecteurs qui a attiré notre attention sur la particularité de l' implémentation std :: make_pair .
Cependant, c'est le cas lorsque l'on ne sait pas s'il vaut la peine d'améliorer le comportement de l'analyseur. Le fait est que ce code est trop compliqué. Vous devez admettre que ce que le code que nous avons analysé ne mérite pas une enquête aussi détaillée, qui a traîné sur tout un article. Si ce code demande autant d'attention, c'est un très mauvais code.
Il convient ici de rappeler l'article "Les faux positifs sont nos ennemis, mais peuvent encore être vos amis ". La publication n'est pas la nôtre, mais nous sommes d'accord avec elle.
C'est peut-être le cas même. L'avertissement peut être faux, mais il indique un meilleur endroit pour refactoriser. Il suffit d'écrire quelque chose comme ceci:
auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.push_back(
std::make_pair(std::move(buffer), delta));
Ou vous pouvez rendre le code encore meilleur dans cette situation en utilisant la méthode emplace_back :
auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.emplace_back(std::move(buffer), delta);
Ce code créera l'objet std :: pair résultant dans le conteneur "in place", en contournant la création d'un objet temporaire et en le déplaçant vers le conteneur. D'ailleurs, l'analyseur PVS-Studio suggère d'effectuer un tel remplacement en émettant un avertissement V823 à partir de l'ensemble de règles pour les micro-optimisations de code.
Le code deviendra sans ambiguïté plus simple et plus clair pour tout lecteur et analyseur. Il n'y a aucun intérêt à regrouper autant d'actions que possible dans une seule ligne de code.
Oui, dans ce cas, il a eu de la chance et il n'y a pas d'erreur. Mais il est peu probable que l'auteur, en écrivant ce code, ait gardé tout ce dont nous avons discuté dans sa tête. Très probablement, c'est la chance qui a joué. Et une autre fois, vous n'aurez peut-être pas de chance.
Conclusion
Donc, nous avons compris qu'il n'y avait pas de véritable erreur. L'analyseur génère un faux positif. Peut-être supprimerons-nous l'avertissement uniquement pour de tels cas, mais peut-être pas. Nous y réfléchirons plus tard. Après tout, c'est un cas plutôt rare, et le code où les arguments sont évalués avec des effets secondaires est dangereux en général, et il vaut mieux l'éviter. Il vaut la peine de refactoriser au moins à des fins préventives.
Afficher le code:
Foo(std::move(buffer), line_buffer - buffer.get());
facile à casser en changeant quelque chose ailleurs dans le programme. Un code comme celui-ci est difficile à maintenir. Et c'est aussi désagréable en ce sens que cela peut donner une fausse impression que tout fonctionne correctement. En fait, ce n'est qu'une coïncidence, et tout peut casser si vous modifiez les paramètres du compilateur ou d'optimisation.
Simplifiez votre code!
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: Andrey Karpov. L'analyseur de code est faux. Vive l'analyseur! ...