Clever Geek Handbook

コードアナライザーが間違っている、アナライザーを長持ちさせる

Foo（std :: move（buffer）、line_buffer --buffer.get（））;






C ++言語の1つの式に多くのアクションを組み合わせるのは悪いことです。そのようなコードは理解しにくく、保守しにくく、間違いもしやすいからです。たとえば、関数の引数を評価するときにさまざまなアクションを組み合わせてバグを作成します。コードはシンプルでわかりやすいものにするという古典的な推奨事項に同意します。そして、正式にはPVS-Studioアナライザーが間違っているが、実用的な観点からは、コードを変更する必要があるという興味深いケースを考えてみましょう。



引数の評価順序



ここでお話しするのは、「うさぎの穴の深さやPVS-StudioでのC ++でのインタビュー」の記事で書いた、引数の計算順序に関する古い話の続きです。



簡単なエッセンスは以下の通りです。関数引数が評価される順序は、不特定の動作です。この規格は、コンパイラ開発者が引数を評価しなければならない正確な順序を指定していません。たとえば、左から右（Clang）または右から左（GCC、MSVC）です。 C ++ 17標準より前では、引数の評価時に副作用が発生すると、未定義の動作が発生する可能性がありました。



C ++ 17標準の出現により、状況は改善されました。引数とその副作用の計算は、前の引数のすべての計算と副作用が実行された瞬間からのみ実行されるようになりました。ただし、これはエラーの余地がないことを意味するものではありません。



簡単なテストプログラムを考えてみましょう。 



#include <cstdio>
int main()
{
  int i = 1;
  printf("%d, %d\n", i, i++);
  return 0;
}


このコードは何を出力しますか？答えはまだコンパイラ、バージョン、および気分に依存します。コンパイラによっては、「1、1」と「2、1」の両方を印刷できます。実際、Compiler Explorerを使用すると、次の結果が得られます。



  • Clang 11.0.0でコンパイルされたプログラムは、「1、1」を生成します。
  • GCC 10.2でコンパイルされたプログラムは、「2、1」を生成します。


このプログラムには未定義の動作はありませんが、未指定の動作（引数の評価の順序）があります。



CSVパーサープロジェクトのコード



記事「ヘッダーのみのC ++ライブラリのコレクションのチェック（awesome-hpp）」で説明したCSVパーサープロジェクトのコードスニペットに戻りましょう。



パーサーと私は、引数を異なる順序で評価できることを知っています。したがって、アナライザー、およびその後の私自身は、このコードが誤っていると見なしました。 



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()));


PVS-Studioの警告：V769'line_buffer --buffer.get（） '式の' buffer.get（） 'ポインターがnullptrと等しい。結果の値は無意味であり、使用しないでください。csv.hpp 4957



実際、私たちは両方とも間違っており、間違いはありません。ニュアンスについてはさらに話しますが、とりあえず簡単なものから始めましょう。



それでは、次のようなコードを書くことがなぜ危険なのかを見てみましょう。 



Foo(std::move(buffer), line_buffer - buffer.get());


答えは推測できると思います。結果は、引数が評価される順序によって異なります。次の合成コードでこれを考慮してください。 



#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;
}


コンパイラエクスプローラをもう一度使用して、さまざまなコンパイラによってコンパイルされたこのプログラムの出力を見てみましょう。



Clangコンパイラ11.0.0。結果： 



23387846
22


GCCコンパイラ10.2。結果： 



22
26640070


結果を期待しており、そのように書くことはできません。これはまさにPVS-Studioアナライザーが警告するものです。



これに終止符を打ちたいのですが、すべてがもう少し複雑です。実際には、引数を値で渡すことについて話しているので、std :: make_pair関数テンプレートをインスタンス化すると、すべてが異なります。ニュアンスを掘り下げて、この場合にPVS-Studioが間違っている理由を調べてみましょう。



std :: make_pair



cppreference Webサイトを見て、std :: make_pair関数テンプレートがどのように変更されたかを見てみましょう。



C ++ 11まで：
テンプレート<クラスT1、クラスT2>

std ::ペア<T1、T2> make_pair（T1 t、T2 u）;
C ++ 11以降、C ++ 14まで：
テンプレート<クラスT1、クラスT2>

std ::ペア<V1、V2> make_pair（T1 && t、T2 && u）;
C ++ 14以降：
template< class T1, class T2 >

constexpr std::pair<V1,V2> make_pair( T1&& t, T2&& u );
ご覧のとおり、昔々、std :: make_pairは値によって引数を受け入れました。その時点でstd :: unique_ptrが存在していた場合、上記のコードは実際には正しくありません。このコードが機能するかどうかは運の問題でした。もちろん、実際には、std :: unique_ptrがstd :: auto_ptrの代わりとしてC ++ 11に登場したため、このような状況は発生しませんでした。



私たちの時代に戻りましょう。 C ++ 11標準のバージョンから、コンストラクターは移動セマンティクスの使用を開始しました。std :: moveは実際には何も移動せず、オブジェクトを右辺値参照に変換するだけ



であるという微妙な点があります。これにより、std :: make_pairはポインタを新しいstd :: unique_ptrに渡し、nullptrを元のスマートポインタに残します。ただし、このポインタの受け渡しは、std :: make_pairに入るまで発生しません。その時までに、line_buffer --buffer.get（）をすでに計算しており、すべてが正常になります。つまり、buffer.get（）の呼び出しは、いつ発生しても、評価時にnullptrを返すことはできません。



複雑な説明でごめんなさい。肝心なのは、このコードは完全に有効であるということです。実際、この場合、PVS-Studio静的アナライザーは誤ったアラームを出しました。しかし、私たちのチームは、そのような状況のためにアナライザーのロジックを急いで変更する必要があるかどうか確信がありません。



王は死んだ、王は長生きする！



記事に 記載されている操作が誤りであることが判明しました。std :: make_pair実装の特殊性に注意を向けてくれた読者の1人に感謝します。



ただし、これは、アナライザーの動作を改善する価値があるかどうかわからない場合に当てはまります。重要なのは、このコードが複雑すぎるということです。私たちが分析したコードは、記事全体を引きずったような詳細な調査に値しないことを認めなければなりません。このコードに非常に注意が必要な場合、それは非常に悪いコードです。



ここで、「誤検知は私たちの敵ですが、それでもあなたの友達かもしれない」という記事を思い出すのが適切です。出版物は私たちのものではありませんが、私たちはそれに同意します。



これはおそらくまさにその通りです。警告は誤っている可能性がありますが、リファクタリングするのに適した場所を示しています。このようなものを書くだけで十分です： 



auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.push_back(
  std::make_pair(std::move(buffer), delta));


または、emplace_backメソッドを使用して、この状況でコードをさらに改善することができます。 



auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.emplace_back(std::move(buffer), delta);


このコードは、一時オブジェクトの作成とそれをコンテナに移動することをバイパスして、結果のstd ::ペアオブジェクトをコンテナの「インプレース」に作成します。ちなみに、PVS-Studioアナライザーは、コードのマイクロ最適化の一連のルールから警告V823を発行することにより、このような置換を行うことを提案しています。



コードは、どのリーダーやアナライザーにとっても明確に単純で明確になります。1行のコードにできるだけ多くのアクションを詰め込むことにメリットはありません。



はい、この場合は幸運で、エラーはありません。しかし、著者がこのコードを書いているときに、私たちが議論したすべてのことを頭の中で保持している可能性は低いです。おそらく、プレーしたのは運でした。そしてまた別の時にはあなたは幸運ではないかもしれません。



結論



したがって、実際のエラーはないことがわかりました。アナライザーは誤検知を生成します。おそらく、そのような場合にのみ警告を削除するか、そうでないかもしれません。後で考えます。結局のところ、これはかなりまれなケースであり、引数が副作用で評価されるコードは一般に危険であり、それを回避することをお勧めします。少なくとも予防目的でリファクタリングする価値があります。



コードを表示： 



Foo(std::move(buffer), line_buffer - buffer.get());


プログラムの他の場所で何かを変更することで簡単に壊すことができます。このようなコードは維持するのが難しいです。また、すべてが正常に機能しているという誤った感覚を与える可能性があるという点でも不快です。実際、これは単なる偶然であり、コンパイラまたは最適化の設定を変更すると、すべてが壊れることがあります。



コードを簡単にしましょう！









この記事を英語を話す聴衆と共有したい場合は、翻訳リンクを使用してください：AndreyKarpov。コードアナライザーが間違っています。アナライザーは長持ちします！..。


More articles:


All Articles