Durante todo o período de sua existência, as pessoas fizeram um esforço tremendo para estudar quase toda a área do céu estrelado. Até o momento, examinamos centenas de milhares de asteróides, cometas, nebulosas e estrelas, galáxias e planetas. Para ver toda essa beleza, não é necessário sair de casa e comprar um telescópio. Você pode instalar o Stellarium - um planetário virtual no seu computador e olhar para o céu noturno, confortavelmente deitado no sofá ... Mas é confortável? Para descobrir a resposta a esta pergunta, verificaremos o Stellarium quanto a erros no código do computador.
Um pouco sobre o projeto ...
De acordo com a
descrição no site da Wikipedia, o
Stellarium é um planetário virtual de código aberto disponível para Linux, Mac OS X, Microsoft Windows, Symbian, Android e iOS, além do MeeGo. O programa usa as tecnologias OpenGL e Qt para criar um céu realista em tempo real. Com o Stellarium, você pode ver o que pode ver com um telescópio médio e até grande. O programa também fornece observações de eclipses solares e o movimento de cometas.
O Stellarium foi criado pelo programador francês Fabian Chereau, que lançou o projeto no verão de 2001. Outros desenvolvedores de destaque incluem Robert Spearman, Johannes Gadzhozik, Matthew Gates, Timothy Reeves, Bogdan Marinov e Johan Meeris, responsável pela obra de arte.
... e sobre o analisador
A análise do projeto foi realizada no analisador de código estático PVS-Studio. Esta é uma ferramenta para detectar erros e possíveis vulnerabilidades no código fonte dos programas escritos em C, C ++ e C # (em breve em Java!). É executado no Windows, Linux e macOS. Ele foi desenvolvido para aqueles que precisam melhorar a qualidade de seu código.
A análise foi bastante simples. Primeiro,
baixei o projeto Stellarium do GitHub e instalei todos os pacotes necessários para a montagem. Como o projeto foi criado usando o Qt Creator, usei o sistema de rastreamento de inicialização do compilador, incorporado à versão
autônoma do analisador. Lá, você pode visualizar o relatório de análise concluído.
Novos leitores e usuários do
Stellarium podem ter se perguntado: por que o unicórnio aparece no título do artigo e como ele está relacionado à análise de código? Eu respondo: sou um dos desenvolvedores do PVS-Studio, e o unicórnio é nosso mascote travesso favorito. Então suba!
Espero que, graças a este artigo, os leitores aprendam algo novo por si mesmos, e os desenvolvedores do Stellarium possam eliminar alguns erros e melhorar a qualidade do código.
Traga seu café com um croissant de ar e sinta-se confortável, porque estamos indo para a parte mais interessante - uma visão geral dos resultados da análise e análise de erros!
Condições suspeitas
Para maior prazer de leitura, sugiro que não olhe diretamente para o aviso do analisador, mas tente aqui e mais adiante encontrar você mesmo os erros.
void QZipReaderPrivate::scanFiles() { ....
Aviso do PVS-Studio: V654 A condição 'start_of_directory == - 1' do loop é sempre verdadeira. qzip.cpp 617
Você conseguiu encontrar um erro? Se sim, então elogie.
O erro está na condição do
loop while . É sempre verdade, pois a variável
start_of_directory não muda no corpo do loop. Muito provavelmente, o ciclo não será eterno, pois contém
retorno e
quebra , mas esse código parece estranho.
Parece-me que no código eles esqueceram de fazer a atribuição
start_of_directory = pos no local em que a assinatura está sendo verificada. Então a declaração de
quebra é talvez supérflua. Nesse caso, o código pode ser reescrito assim:
int i = 0; int start_of_directory = -1; EndOfDirectory eod; while (start_of_directory == -1) { const int pos = device->size() - int(sizeof(EndOfDirectory)) - i; if (pos < 0 || i > 65535) { qWarning() << "QZip: EndOfDirectory not found"; return; } device->seek(pos); device->read((char *)&eod, sizeof(EndOfDirectory)); if (readUInt(eod.signature) == 0x06054b50) start_of_directory = pos; ++i; }
No entanto, não tenho certeza de como deve ser o código. É melhor que os próprios desenvolvedores do projeto analisem essa parte do programa e façam as alterações necessárias.
Outra condição estranha:
class StelProjectorCylinder : public StelProjector { public: .... protected: .... virtual bool intersectViewportDiscontinuityInternal(const Vec3d& capN, double capD) const { static const SphericalCap cap1(1,0,0); static const SphericalCap cap2(-1,0,0); static const SphericalCap cap3(0,0,-1); SphericalCap cap(capN, capD); return cap.intersects(cap1) && cap.intersects(cap2) && cap.intersects(cap2); } };
PVS-Studio Warning: V501 Existem sub-expressões idênticas 'cap.intersects (cap2)' à esquerda e à direita do operador '&&'. StelProjectorClasses.hpp 175
Como você provavelmente já adivinhou, o erro está na última linha da função: o programador cometeu um erro de digitação e, no final, descobriu que a função retorna o resultado independentemente do valor de
cap3 .
Esse tipo de erro é extremamente comum: em quase todos os projetos testados, encontramos erros de digitação relacionados a nomes do formulário
nome1 e
nome2 e similares. Normalmente, esses erros estão relacionados à copiar e colar.
Essa instância de código é um excelente exemplo de outro padrão de erro comum, para o qual realizamos um mini-estudo separado. Meu colega Andrei Karpov chamou o "
efeito de última linha ". Se você não estiver familiarizado com este material, sugiro abrir uma guia no navegador para ler mais tarde, mas por enquanto, vamos continuar.
void BottomStelBar::updateText(bool updatePos) { .... updatePos = true; .... if (location->text() != newLocation || updatePos) { updatePos = true; .... } .... if (fov->text() != str) { updatePos = true; .... } .... if (fps->text() != str) { updatePos = true; .... } if (updatePos) { .... } }
Avisos do PVS-Studio:- V560 Uma parte da expressão condicional é sempre verdadeira: updatePos. StelGuiItems.cpp 732
- A expressão V547 'updatePos' sempre é verdadeira. StelGuiItems.cpp 831
- V763 O parâmetro 'updatePos' é sempre reescrito no corpo da função antes de ser usado. StelGuiItems.cpp 690
O valor do parâmetro
updatePos é sempre substituído antes de ser usado, ou seja, a função funcionará da mesma forma, independentemente do valor passado para ela.
Parece estranho, não é? Em todos os lugares em que o parâmetro
updatePos está
envolvido , é
verdade . Isso significa que as condições
if (location-> text ()! = NewLocation || updatePos) e
if (updatePos) sempre serão verdadeiras.
Outro trecho:
void LandscapeMgr::onTargetLocationChanged(StelLocation loc) { .... if (pl && flagEnvironmentAutoEnabling) { QSettings* conf = StelApp::getInstance().getSettings(); setFlagAtmosphere(pl->hasAtmosphere() & conf->value("landscape/flag_atmosphere", true).toBool()); setFlagFog(pl->hasAtmosphere() & conf->value("landscape/flag_fog", true).toBool()); setFlagLandscape(true); } .... }
Avisos do PVS-Studio:- V792 A função 'toBool' localizada à direita do operador '&' será chamada independentemente do valor do operando esquerdo. Talvez seja melhor usar '&&'. LandscapeMgr.cpp 782
- V792 A função 'toBool' localizada à direita do operador '&' será chamada independentemente do valor do operando esquerdo. Talvez seja melhor usar '&&'. LandscapeMgr.cpp 783
O analisador detectou uma expressão suspeita nos argumentos para as
funções setFlagAtmosphere e
setFlagFog . De fato: nos dois lados do operador de bit
e existem valores do tipo
bool . Em vez do operador
& , você deve usar o operador
&& , e agora vou explicar o porquê.
Sim, o resultado dessa expressão sempre estará correto. Antes de usar o “e” bit a bit, os dois operandos serão promovidos para
int . No C ++, essa conversão é
inequívoca : false é convertido em 0 e true é convertido em 1. Portanto, o resultado dessa expressão será o mesmo que se o operador
&& fosse usado.
Mas há uma nuance. Ao calcular o resultado da operação
&& , é utilizado o chamado "cálculo lento". Se o valor do operando esquerdo for
falso , o valor correto nem será calculado, porque o "e" lógico, em qualquer caso, retornará
falso . Isso é feito para economizar recursos de computação e permite que você escreva projetos mais complexos. Por exemplo, você pode verificar se o ponteiro não é nulo e, se houver, desreferenciá-lo para executar uma verificação adicional. Exemplo:
if (ptr && ptr-> foo ()) .
Esse “cálculo preguiçoso” não é realizado usando o operador bit a bit
& : expression
conf-> value ("...", true) .toBool () será avaliado sempre, independentemente do valor
pl-> hasAtmosphere () .
Em casos raros, isso é feito de propósito. Por exemplo, se a computação do operando certo tiver "efeitos colaterais", o resultado será usado posteriormente. Também não é muito bom fazê-lo, porque complica a compreensão do código e a manutenção dele. Além disso, a ordem de cálculo dos operandos
e não
está definida, portanto, em alguns casos, usando esses "truques", você pode obter um comportamento indefinido.
Se você precisar salvar efeitos colaterais - faça-o em uma linha separada e salve o resultado em uma variável separada. As pessoas que trabalharão com esse código no futuro serão gratas a você :)
Prosseguimos para o próximo tópico.
Manuseio incorreto de memória
Vamos começar o tópico da memória dinâmica com este fragmento:
GLUEShalfEdge* __gl_meshMakeEdge(GLUESmesh* mesh) { GLUESvertex* newVertex1 = allocVertex(); GLUESvertex* newVertex2 = allocVertex(); GLUESface* newFace = allocFace(); GLUEShalfEdge* e; if ( newVertex1 == NULL || newVertex2 == NULL || newFace == NULL) { if (newVertex1 != NULL) { memFree(newVertex1); } if (newVertex2 != NULL) { memFree(newVertex2); } if (newFace != NULL) { memFree(newFace); } return NULL; } e = MakeEdge(&mesh->eHead); if (e == NULL) { return NULL; } MakeVertex(newVertex1, e, &mesh->vHead); MakeVertex(newVertex2, e->Sym, &mesh->vHead); MakeFace(newFace, e, &mesh->fHead); return e; }
Avisos do PVS-Studio:- V773 A função foi encerrada sem liberar o ponteiro 'newVertex1'. É possível um vazamento de memória. mesh.c 312
- V773 A função foi encerrada sem liberar o ponteiro 'newVertex2'. É possível um vazamento de memória. mesh.c 312
- V773 A função foi encerrada sem liberar o ponteiro 'newFace'. É possível um vazamento de memória. mesh.c 312
A função aloca memória para várias estruturas e a passa para os ponteiros
newVertex1 ,
newVertex2 (nomes interessantes, certo?) E
newFace . Se um deles for zero, toda a memória reservada dentro da função será liberada, após o que o fluxo de controle sairá da função.
O que acontece se a memória das três estruturas for alocada corretamente e a função
MakeEdge (& mesh-> eHead) retornar
NULL ? O fluxo de controle alcançará o segundo
retorno .
Como os ponteiros
newVertex1 ,
newVertex2 e
newFace são variáveis locais, eles deixarão de existir após a saída da função. Mas a liberação da memória que lhes pertencia não acontecerá. Ele permanecerá reservado, mas não teremos mais acesso a ele.
Tais situações são chamadas de vazamento de memória. Um cenário típico com esse erro: com o uso prolongado do programa, ele começa a consumir mais e mais RAM, até o esgotamento.
Deve-se notar que, neste exemplo, o terceiro
retorno não
é errado. As
funções MakeVertex e
MakeFace transferem os endereços da memória alocada para outras estruturas de dados, delegando assim a responsabilidade por sua liberação.
A próxima falha está no método, que leva 90 linhas. Por conveniência, reduzi, deixando apenas áreas problemáticas.
void AstroCalcDialog::drawAngularDistanceGraph() { .... QVector<double> xs, ys; .... }
Apenas uma linha restante. Deixe-me dar uma dica: esta é a única menção aos objetos
xs e
ys .
Avisos do PVS-Studio:- O objeto V808 'xs' do tipo 'QVector' foi criado, mas não foi utilizado. AstroCalcDialog.cpp 5329
- O objeto V808 'ys' do tipo 'QVector' foi criado, mas não foi utilizado. AstroCalcDialog.cpp 5329
Os vetores
xs e
ys são criados, mas não são usados em nenhum lugar. Acontece que toda vez que você usa o método
drawAngularDistanceGraph ,
ocorre uma criação e exclusão extras de um contêiner vazio. Acho que esse anúncio permaneceu no código após a refatoração. Obviamente, isso não é um erro, mas você deve remover o código extra.
Moldes de tipo estranho
Outro exemplo depois de um pouco de formatação é assim:
void SatellitesDialog::updateSatelliteData() { ....
Para entender qual é o erro, você precisará examinar os protótipos
dos construtores da classe Qcolor :
Avisos do PVS-Studio:- V674 O literal '0.4' do tipo 'double' está sendo implicitamente convertido para o tipo 'int' enquanto chama a função 'QColor'. Inspecione o primeiro argumento. SatellitesDialog.cpp 413
- V674 O literal '0.4' do tipo 'double' está sendo implicitamente convertido para o tipo 'int' enquanto chama a função 'QColor'. Inspecione o segundo argumento. SatellitesDialog.cpp 413
- V674 O literal '0.4' do tipo 'double' está sendo implicitamente convertido para o tipo 'int' enquanto chama a função 'QColor'. Inspecione o terceiro argumento. SatellitesDialog.cpp 413
A classe
Qcolor não possui construtores que aceitam o tipo
double ; portanto, os argumentos no exemplo serão implicitamente convertidos em
int . Isso faz com que os campos
r ,
g ,
b do objeto
buttonColor tenham um valor
0 .
Se o programador pretendia criar um objeto a partir de valores do tipo
double , ele deveria usar um construtor diferente.
Por exemplo, você pode usar um construtor que aceite
Qrgb escrevendo:
buttonColor = QColor(QColor::fromRgbF(0.4, 0.4, 0.4));
Poderia ter sido feito de maneira diferente. Qt usa valores reais no intervalo [0,0, 1,0] ou valores inteiros no intervalo [0, 255] para indicar cores RGB.
Portanto, o programador pode traduzir os valores de real para inteiro escrevendo assim:
buttonColor = QColor((int)(255 * 0.4), (int)(255 * 0.4), (int)(255 * 0.4));
ou apenas
buttonColor = QColor(102, 102, 102);
Você está entediado? Não se preocupe: há erros mais interessantes pela frente.
"Unicórnio no espaço". Vista do Stellarium.Outros erros
No final, deixei vocês mais deliciosos :) Vamos falar com um deles.
HipsTile* HipsSurvey::getTile(int order, int pix) { .... if (order == orderMin && !allsky.isNull()) { int nbw = sqrt(12 * 1 << (2 * order)); int x = (pix % nbw) * allsky.width() / nbw; int y = (pix / nbw) * allsky.width() / nbw; int s = allsky.width() / nbw; QImage image = allsky.copy(x, y, s, s); .... } .... }
Aviso do PVS-Studio: V634 A prioridade da operação '*' é maior que a da operação '<<'. É possível que parênteses sejam usados na expressão. StelHips.cpp 271
Bem, você conseguiu detectar o erro? Considere a expressão
(12 * 1 << (2 * ordem)) em mais detalhes. O analisador lembra que a operação '
* ' tem uma prioridade mais alta que a operação de troca de bits '
<< '. É fácil ver que a multiplicação de
12 por
1 não faz sentido e os colchetes em torno de
2 * da ordem não
são necessários.
, : int nbw = sqrt(12 * (1 << 2 * order)); <i>12 </i> .
Nota Além disso, quero observar que, se o valor do operando direito '
<< ' for maior ou igual ao número de bits do operando esquerdo, o resultado não será definido. Como literais numéricos são
int por padrão, que leva
32 bits, o valor do parâmetro
order não deve exceder
15 . Caso contrário, a avaliação da expressão pode resultar em comportamento indefinido.
Nós continuamos. O método abaixo é muito confuso, mas tenho certeza de que um leitor sofisticado lidará com a detecção de erros :)
QCPRange QCPStatisticalBox:: getKeyRange(bool& foundRange, SignDomain inSignDomain) const { foundRange = true; if (inSignDomain == sdBoth) { return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); } else if (inSignDomain == sdNegative) { if (mKey + mWidth * 0.5 < 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey < 0) return QCPRange(mKey - mWidth * 0.5, mKey); else { foundRange = false; return QCPRange(); } } else if (inSignDomain == sdPositive) { if (mKey - mWidth * 0.5 > 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey > 0) return QCPRange(mKey, mKey + mWidth * 0.5); else { foundRange = false; return QCPRange(); } } foundRange = false; return QCPRange(); }
PVS-Studio Warning: V779 Código inacessível detectado. É possível que haja um erro. qcustomplot.cpp 19512.
O fato é que todos os ramos
if ... else têm um
retorno . Portanto, o fluxo de controle nunca atinge as duas últimas linhas.
Em geral, este exemplo será executado normalmente e funcionará corretamente. Mas a presença de código inacessível por si só é um sinal. Nesse caso, indica a estrutura incorreta do método, o que complica bastante a legibilidade e a compreensibilidade do código.
Esse fragmento de código deve ser refatorado, obtendo uma função mais elegante na saída. Por exemplo, assim:
QCPRange QCPStatisticalBox:: getKeyRange(bool& foundRange, SignDomain inSignDomain) const { foundRange = true; switch (inSignDomain) { case sdBoth: { return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); break; } case sdNegative: { if (mKey + mWidth * 0.5 < 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey < 0) return QCPRange(mKey - mWidth * 0.5, mKey); break; } case sdPositive: { if (mKey - mWidth * 0.5 > 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey > 0) return QCPRange(mKey, mKey + mWidth * 0.5); break; } } foundRange = false; return QCPRange(); }
O último em nossa análise será o erro que eu mais gostei. O código do local do problema é curto e simples:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) : distance(0.0f), sDistance(0.0f) { Plane(v1, v2, v3, SPolygon::CCW); }
Você notou algo suspeito? Nem todo mundo pode :)
PVS-Studio Warning: V603 O objeto foi criado, mas não está sendo usado. Se você deseja chamar o construtor, 'this-> Plane :: Plane (....)' deve ser usado. Plane.cpp 29
O programador esperava que alguns dos campos do objeto fossem inicializados dentro do construtor aninhado, mas acabou assim: quando o construtor
Plane (Vec3f & v1, Vec3f & v2, Vec3f & v3) é chamado, um objeto temporário sem nome é criado dentro dele, que é imediatamente excluído. Como resultado, uma parte do objeto permanece não inicializada.
Para que o código funcione corretamente, você deve usar o recurso conveniente e seguro do C ++ 11 - o construtor de delegação:
Plane::Plane(Vec3f& v1, Vec3f& v2, Vec3f& v3) : Plane(v1, v2, v3, SPolygon::CCW) { distance = 0.0f; sDistance = 0.0f; }
Mas se você usar o compilador para versões mais antigas da linguagem, poderá escrever assim:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) : distance(0.0f), sDistance(0.0f) { this->Plane::Plane(v1, v2, v3, SPolygon::CCW); }
Ou então:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) : distance(0.0f), sDistance(0.0f) { new (this) Plane(v1, v2, v3, SPolygon::CCW); }
Noto que os dois últimos métodos são
muito perigosos . Portanto, você deve ter muito cuidado e entender bem como esses métodos funcionam.
Conclusão
Que conclusões podem ser tiradas sobre a qualidade do código do Stellarium? Honestamente, não houve muitos erros. Além disso, em todo o projeto, não encontrei um único erro no qual o código esteja vinculado a um comportamento indefinido. Para o projeto de código-fonte aberto, a qualidade do código acabou sendo de alto nível, para a qual tiro meu chapéu para os desenvolvedores. Vocês são ótimos! Fiquei satisfeito e interessado em revisar seu projeto.
E o próprio planetário - eu o uso com bastante frequência. Infelizmente, morando em uma cidade, raramente consigo desfrutar de um céu noturno claro, e o Stellarium me permite estar em qualquer lugar do mundo sem me levantar do sofá. É realmente confortável!
Eu gosto especialmente do modo "Constellation art". A visão das figuras enormes cobrindo o céu inteiro em uma dança estranha é de tirar o fôlego.
"Uma dança estranha." Vista do Stellarium.Os terráqueos tendem a cometer erros, e não há nada de vergonhoso no fato de esses erros vazarem no código. Para isso, são desenvolvidas ferramentas de análise de código, como o PVS-Studio. Se você é um dos terráqueos -
como ele, sugiro que faça o
download e experimente você mesmo .
Espero que você esteja interessado em ler meu artigo e tenha aprendido algo novo e útil para si mesmo. E desejo aos desenvolvedores uma correção precoce dos erros encontrados.
Assine nossos canais e fique atento às novidades do mundo da programação!
Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: George Gribkov.
Into Space Again: como o unicórnio visitou o Stellarium