Cプリプロセッサの判定を強化

[beru]

PR の目的

CCppPreprocessMng::ScanLine の処理を変更して #elif にも対応するのが目的です。

カテゴリ

• 機能追加

PR の背景

この処理はアウトライン解析ウィンドウに表示するC/C++関数リストを作成する処理で使用されます。

なおCプリプロセッサの色付け処理は（残念ながら）従来のままで強化されません。このPRで変更した箇所の処理が色付けには関わっていない為です。

PR のメリット

Cプリプロセッサの #elif に対応したアウトライン解析が行われるようになります。

PR のデメリット (トレードオフとかあれば)

CCppPreprocessMng::ScanLine の実装が複雑化して読み取りにくくなります。

仕様・動作説明

従来実装では #elif に対応していなかったので対応するようにしました。

実装は色々と変えていますが、

• 対応するCプリプロセッサディレクティブの種類が増えたので、それを読み取る処理の記述を変えました。
• CCppPreprocessMng クラスに m_activated というメンバーを追加しています。各レベルでC/C++での走査を行う場所が有ったかどうかの記録用です。この変数を追加せずに色々なケースで正しく処理させる方法が思いつきませんでした。
  • 従来実装から存在する m_enablebuf というフラグ的な変数の使用と合わせてかなり理解しづらい記述になっていると思います。
• #if defined(...) や #if !defined(...) の記述には相変わらず対応出来ていません。

PR の影響範囲

アウトライン解析ウィンドウに表示するC/C++関数リストを作成する処理で使用されるCプリプロセッサ判定処理

テスト内容

テスト1

手順

テスト用に cpp_test.cpp.txt を使用しました。拡張子が cpp だと添付出来ないので偽装して txt にしています。

小さいファイルなのでダウンロードしなくても内容が見れるようにしました。

内容

#if 0 // 0
void func_0() {}
#if 0 // 0_0
void func_0_0() {}
#elif 0 // 0_1
void func_0_1() {}
#else // 0_2
void func_0_2() {}
#endif

#elif 0 // 1
void func_1() {}
#if 0 // 1_0
void func_1_0() {}
#elif 0 // 1_1
void func_1_1() {}
#else // 1_2
void func_1_2() {}
#endif

#else // 2
void func_2() {}
#if 0 // 2_0
void func_2_0() {}
#elif 0 // 2_1
void func_2_1() {}
#else  // 2_2
void func_2_2() {}
#endif

#endif

上記のテキスト中のディレクティブの定数を変更してからF11 キーを押してアウトライン解析の表示が期待通りかどうかを確認してテストしました。

関連 issue, PR

#1084

参考資料

https://docs.microsoft.com/en-us/cpp/preprocessor/hash-if-hash-elif-hash-else-and-hash-endif-directives-c-cpp

https://en.wikipedia.org/wiki/C_preprocessor

[sonarcloud]

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

0.0% Coverage
0.0% Duplication

[beru]

このPRの変更を入れても色々と問題点は残ります。

例えば下記のようなテキストでアウトライン解析すると

#if 0
/*
#endif
void func0() {}
*/
#endif

func0 がリストに表示されますが、本来はC言語の /* と */ で囲うコメント中の記述なので登場するべきではないと思います。

[AppVeyorBot]

✅ Build sakura 1.0.3705 completed (commit 088892d98c by @beru)

[berryzplus]

アウトライン解析機能はどうしましょうか。

コードの構成的にバグが発生しやすい機能です。
今回の提案内容も「実装漏れ」というバグの対策だと思います。
しかも、発見した既知のバグの対応を見送るんですよね。
誰がやってもなんかしらのバグが残る、ということが問題っす。

#ifに対応してるのに#elif に対応しないのは明らかな手落ちなので、対応したほうがよい気がします。
PRに対してはコメントなしでapproveでいいと思います。
既知の不具合は別途で対応したらよいはずです。

根本対策は、ドキュメント構造からアウトラインを読み取る部分と、読み取ったアウトラインを画面表示する部分を完全に分解し、ドキュメント構造からアウトラインを読み取る部分のコードを「構文規則」から自動生成するコードに置き換えることだと思っています。

issue立てると何年も残りそうな話題なので一旦コメントにします。

どう思います？

[beru]

コードの構成的にバグが発生しやすい機能です。
今回の提案内容も「実装漏れ」というバグの対策だと思います。
しかも、発見した既知のバグの対応を見送るんですよね。
誰がやってもなんかしらのバグが残る、ということが問題っす。

どういう実装をしても広義のバグにカテゴライズされてしまうと「お前はいっつもバグを入れやがってこの〇ータ〇ンが！」というパワハラ罵声が脳内で響いてくるので逃げ腰になってしまいそうですが、まぁ冗談はおいておいて…。

まず「実装漏れ」がバグかどうかというのはケースバイケースだと思います。また意図的に決めて実装していなかったのか、それとも考慮漏れや忘れで実装がされなかったのかという違いもあると思います。とはいえ過去の事はおいておいて…。

どこまで対応して何には対応しないのかの計画を立てて作っていく方法もありますが、とはいえそういう文書化が必須化というとそうでもないとは思います。はい、面倒事の回避です。

#ifに対応してるのに#elif に対応しないのは明らかな手落ちなので、対応したほうがよい気がします。
PRに対してはコメントなしでapproveでいいと思います。
既知の不具合は別途で対応したらよいはずです。

自分がいまのとこ把握している範囲だと /* */ 範囲コメントや #if defined や #if !defined には未対応です。あとまぁ色付け表示もCプリプロセッサにきちんと対応していないですね。このPRで対応しない範囲については新規にIssueを作ってそこのタスクリストに列挙しておくのが良い気がするので、このPRでそれらについて対応しないのでも良いのであれば後でIssueを作っておきます。

このPRがコメントなしでapproveでいいかというと、良し悪しの判断はおいておいてapproveを貰えたらすぐにmergeさせてもらいます。

個人的にはレビューアーが色々と動作確認をしてもらえると助かりますが、レビューアーの視点だとそんな事をやるのは面倒だと思うので、やはりPR作成者がテストデータとテストケースを用意して変更した実装に不具合が無さそうだという事の説得力を持たせるべきなんでしょうね、実際にやるのは大変なのでやってませんがまぁ開発プロセスの中で手抜きすると不具合はすり抜けていくものなのできちんとしたテストはどこかで用意したいですね。つまり先延ばしです。

根本対策は、ドキュメント構造からアウトラインを読み取る部分と、読み取ったアウトラインを画面表示する部分を完全に分解し、ドキュメント構造からアウトラインを読み取る部分のコードを「構文規則」から自動生成するコードに置き換えることだと思っています。

アウトライン読み取りとその結果の画面表示が分離されていないのかというと処理としては分離されていると思います。アウトライン解析処理は CDocOutline::MakeFuncList_* 系のメソッドですが引数の CFuncInfoArr* pcFuncInfoArr に解析結果を保存しています。その結果を画面表示するのは CDlgFuncList::DoModeless や CDlgFuncList::Redraw なので。

「構文規則」から自動生成に実装を変える事についてですが、手書きと比べて良し悪しはあると思います。なお CDocOutline::MakeFuncList_C メソッドの行数が900行以上あるのは手書きで色々やっているからですが、これだけ長いと内容を把握して手を入れるのが難しくなりますね。

今の実装の CDocOutline クラスは状態らしい状態は保持していないのでクラスではなくて関数としても実装が出来ちゃいますね。とはいえこれの実装内容についてはひとまずおいておきます。

画面表示やキー操作の度にドキュメント全体からアウトラインを読み取る処理を実行したくないので構造化データに結果を保持しておきたいとは思います（メモリ使用量は増大してしまいますが）。今の実装だとアウトライン解析ウィンドウを表示する度にアウトライン解析処理を実行するので、前回実行してから何もドキュメントを変更していなくても再度アウトライン解析ウィンドウを表示するとまた同じ処理の実行が行われます。とはいえドキュメントが小規模な場合はアウトライン解析はすぐ終わるので問題には感じません。

あとドキュメントの内容を変更した際にアウトライン解析ウィンドウの表示内容がリアルタイムで変化しませんが、これはまた別の仕様の話ですね。

[beru]

話は変わりますが CFuncInfoArr クラスってポインタ配列のデータ構造で CFuncInfoArr::AppendData で malloc や realloc を呼び出す作りな事に気づきました。ポインタ配列にせずに std::vector<CFuncInfo> を使うようにした方がメモリの動的確保を頻繁に行わなくなるので良いのではないかと思いました。

[berryzplus]

解析と表示の分離は、現状でできているかちゃんと確認できていないです。
c/c++のアウトライン解析の「表示」に関する修正が過去なんどか入っている気がします。
言語別の表示が実装されているなら「分離できてるとは言えない」と思って書きましたが、
現状で分離できているなら残件は「コード依存」から「ルール依存」への転換をするかどうかだけになります。

[beru]

レビューありがとうございます。マージします。
もし問題が見つかったらPRを作成して修正します。