Es malo combinar muchas acciones en una expresión de C ++, ya que dicho código es difícil de entender, difícil de mantener y también es fácil cometer errores. Por ejemplo, cree un error combinando diferentes acciones al evaluar los argumentos de la función. Estamos de acuerdo con la recomendación clásica de que el código debe ser simple y directo. Y ahora consideraremos un caso interesante, cuando formalmente el analizador PVS-Studio está mal, pero desde un punto de vista práctico, el código aún debe cambiarse.
Orden de evaluación de argumentos
Lo que se contará ahora es una continuación de la vieja historia sobre el orden de cálculo de los argumentos, sobre la que escribimos en el artículo " La profundidad de un agujero de conejo o una entrevista en C ++ en PVS-Studio ".
La breve esencia es la siguiente. El orden en el que se evalúan los argumentos de la función es un comportamiento no especificado. El estándar no especifica el orden exacto en el que los desarrolladores de compiladores deben evaluar los argumentos. Por ejemplo, de izquierda a derecha (Clang) o de derecha a izquierda (GCC, MSVC). Antes del estándar C ++ 17, cuando se producían efectos secundarios al evaluar argumentos, esto podía llevar a un comportamiento indefinido.
Con la llegada del estándar C ++ 17, la situación ha cambiado para mejor: ahora el cálculo del argumento y sus efectos secundarios comenzarán a realizarse solo desde el momento en que se realicen todos los cálculos y efectos secundarios del argumento anterior. Sin embargo, esto no significa que ahora no haya margen de error.
Considere un programa de prueba simple:
#include <cstdio>
int main()
{
int i = 1;
printf("%d, %d\n", i, i++);
return 0;
}
¿Qué imprimirá este código? La respuesta aún depende del compilador, la versión y el estado de ánimo. Dependiendo del compilador, se pueden imprimir tanto "1, 1" como "2, 1". De hecho, al usar el Explorador de compiladores, obtengo los siguientes resultados:
- un programa compilado con Clang 11.0.0 produce "1, 1".
- un programa compilado con GCC 10.2 produce "2, 1".
No hay un comportamiento indefinido en este programa, pero hay un comportamiento no especificado (orden de evaluación de los argumentos).
Código del proyecto CSV Parser
Volvamos al fragmento de código del proyecto CSV Parser, que mencioné en el artículo " Comprobación de la colección de bibliotecas C ++ de solo encabezado (awesome-hpp) ".
El analizador y yo sabemos que los argumentos se pueden evaluar en un orden diferente. Por lo tanto, el analizador, y después yo mismo, consideró que este código era erróneo:
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()));
Advertencia de PVS-Studio: V769 El puntero 'buffer.get ()' en la expresión 'line_buffer - buffer.get ()' es igual a nullptr. El valor resultante no tiene sentido y no debe utilizarse. csv.hpp 4957
En realidad, ambos estamos equivocados y no hay ningún error. Hablaremos más sobre los matices, pero por ahora comencemos con uno simple.
Entonces, veamos por qué es peligroso escribir código como este:
Foo(std::move(buffer), line_buffer - buffer.get());
Creo que puedes adivinar la respuesta. El resultado depende del orden en que se evalúen los argumentos. Considere esto en el siguiente código sintético:
#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;
}
Usemos el Explorador de compiladores nuevamente y veamos el resultado de este programa compilado por diferentes compiladores.
Compilador de Clang 11.0.0. Resultado :
23387846
22
El compilador de GCC 10.2. Resultado :
22
26640070
Esperamos el resultado y no se puede escribir así. Esto es exactamente sobre lo que advierte el analizador PVS-Studio.
Me gustaría acabar con esto, pero todo es un poco más complicado. El hecho es que estamos hablando de pasar argumentos por valor, y al crear una instancia de la plantilla de la función std :: make_pair, todo será diferente. Continuemos profundizando en los matices y descubramos por qué PVS-Studio está equivocado en este caso.
std :: make_pair
Echemos un vistazo al sitio web cppreference y veamos cómo ha cambiado la plantilla de la función std :: make_pair .
Hasta C ++ 11:
plantilla <clase T1, clase T2>Desde C ++ 11, hasta C ++ 14:
std :: par <T1, T2> make_pair (T1 t, T2 u);
plantilla <clase T1, clase T2>Desde C ++ 14:
std :: par <V1, V2> make_pair (T1 && t, T2 && u);
plantilla <clase T1, clase T2>Como puede ver, una vez std :: make_pair aceptaba argumentos por valor. Si std :: unique_ptr existiera en ese momento , entonces el código discutido anteriormente sería incorrecto. Si este código funcionaría o no fue cuestión de suerte. En la práctica, por supuesto, esta situación nunca habría surgido, ya que std :: unique_ptr apareció en C ++ 11 como reemplazo de std :: auto_ptr .
constexpr std :: par <V1, V2> make_pair (T1 && t, T2 && u);
Volvamos a nuestro tiempo. A partir de la versión del estándar C ++ 11, el constructor comenzó a utilizar la semántica de movimiento.
Hay un punto sutil aquí que std :: moveen realidad no mueve nada, solo convierte el objeto en una referencia rvalue . Esto permitirá que std :: make_pair pase el puntero al nuevo std :: unique_ptr , dejando nullptr en el puntero inteligente original. Pero este paso de puntero no sucederá hasta que entremos en std :: make_pair . Para ese momento, ya hemos calculado line_buffer - buffer.get () , y todo estará bien. Es decir, una llamada a buffer.get () no puede devolver nullptren el momento en que se calcula, independientemente de cuándo ocurra exactamente.
Perdón por la descripción complicada. La conclusión es que este código es perfectamente válido. Y de hecho, el analizador estático PVS-Studio dio una falsa alarma en este caso. Sin embargo, nuestro equipo no está seguro de que debamos apresurarnos a realizar cambios en la lógica del analizador para tales situaciones.
El rey esta muerto, larga vida al rey!
Descubrimos que la operación descrita en el artículo resultó ser falsa. Gracias a uno de nuestros lectores que nos llamó la atención sobre la peculiaridad de la implementación std :: make_pair .
Sin embargo, este es el caso cuando no estamos seguros de si vale la pena mejorar el comportamiento del analizador. El caso es que este código es demasiado complicado. Debes admitir que lo que el código que hemos analizado no merece una investigación tan detallada, que arrastró en todo un artículo. Si este código requiere tanta atención, entonces es un código muy malo.
Aquí conviene recordar el artículo "Los falsos positivos son nuestros enemigos, pero aún pueden ser tus amigos ". La publicación no es nuestra, pero estamos de acuerdo con ella.
Este es quizás el caso mismo. La advertencia puede ser falsa, pero apunta a un mejor lugar para refactorizar. Basta con escribir algo como esto:
auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.push_back(
std::make_pair(std::move(buffer), delta));
O puede hacer que el código sea aún mejor en esta situación utilizando el método emplace_back :
auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.emplace_back(std::move(buffer), delta);
Este código creará el objeto std :: pair final en el contenedor "en su lugar", omitiendo la creación de un objeto temporal y moviéndolo al contenedor. Por cierto, el analizador PVS-Studio sugiere hacer tal reemplazo emitiendo una advertencia V823 del conjunto de reglas para microoptimizaciones de código.
El código se volverá claramente más simple y claro para cualquier lector y analizador. No hay ningún mérito en agrupar tantas acciones como sea posible en una línea de código.
Sí, en este caso tuvo suerte y no hay error. Pero es poco probable que el autor, al escribir este código, tuviera en la cabeza todo lo que comentamos. Probablemente, fue la suerte lo que jugó. Y en otra ocasión puede que no tengas suerte.
Conclusión
Entonces, nos dimos cuenta de que no hay un error real. El analizador genera un falso positivo. Quizás eliminemos la advertencia solo para tales casos, pero quizás no. Lo pensaremos más tarde. Después de todo, este es un caso bastante raro, y el código en el que los argumentos se evalúan con efectos secundarios es peligroso en general, y es mejor evitarlo. Vale la pena refactorizar al menos con fines preventivos.
Ver código:
Foo(std::move(buffer), line_buffer - buffer.get());
fácil de romper cambiando algo en otra parte del programa. Un código como este es difícil de mantener. Y también es desagradable porque puede dar una falsa impresión de que todo funciona correctamente. De hecho, esto es solo una coincidencia, y todo puede fallar si cambia la configuración del compilador o la optimización.
¡Haz tu código más fácil!
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace de traducción: Andrey Karpov. El analizador de código está mal. ¡Viva el Analizador! ...