Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

コンパイルテストを導入する #1297

Conversation

berryzplus
Copy link
Contributor

@berryzplus berryzplus commented May 10, 2020

PR の目的

コンパイルテストを導入します。

コンパイルテストは、特定のコード記述がエラーになることを確認する目的のテストです。詳細は経緯に続く・・・。

カテゴリ

  • その他の問題

PR の背景

#1290 で言及した残課題に対処します。

このクラスの主な機能は、暗黙のインスタンス化や許可されない型変換をプログラマにさせないことです。単体テストコードはその性質上、ビルドできることが前提の動作チェックになりますので、そうなった状態の CLayoutInt は実態として int と変わらなかったりします。

サクラエディタには CLayoutIntCLogicInt という 特殊なint型 が存在しています。
これらの型は相互に変換、代入をした場合にコンパイルエラーになるように作られています。
この仕様(=ビルドエラーになる)を確認するテストは、通常の単体テストでは書けません。

現状、x64対応でこれらの型を改造する必要がありそうだと分かっています。
何か修正を加えた時に既存の仕組みを壊していないことの確認ができるようにするために、コンパイルテストを導入したいです。

最初のコミットでは、分かりやすさのために最も基本的なパターンのみをテストするようにしています。

本来はここに書いてある他10パターンとCLayoutIntの特性int型への暗黙変換を許可しないを全てテストする必要があります。

// -- -- -- -- 別種のCStrictIntegerとの演算は絶対許さん(やりたきゃintでも介してください) -- -- -- -- //
private:
template <bool B0,bool B1,bool B2,bool B3> Me& operator += (const CStrictInteger<NOT_STRICT_ID,B0,B1,B2,B3>&);
template <bool B0,bool B1,bool B2,bool B3> Me& operator -= (const CStrictInteger<NOT_STRICT_ID,B0,B1,B2,B3>&);
template <bool B0,bool B1,bool B2,bool B3> Me operator + (const CStrictInteger<NOT_STRICT_ID,B0,B1,B2,B3>&) const;
template <bool B0,bool B1,bool B2,bool B3> Me operator - (const CStrictInteger<NOT_STRICT_ID,B0,B1,B2,B3>&) const;
template <bool B0,bool B1,bool B2,bool B3> Me& operator = (const CStrictInteger<NOT_STRICT_ID,B0,B1,B2,B3>&);
template <bool B0,bool B1,bool B2,bool B3> bool operator < (const CStrictInteger<NOT_STRICT_ID,B0,B1,B2,B3>&) const;
template <bool B0,bool B1,bool B2,bool B3> bool operator <= (const CStrictInteger<NOT_STRICT_ID,B0,B1,B2,B3>&) const;
template <bool B0,bool B1,bool B2,bool B3> bool operator > (const CStrictInteger<NOT_STRICT_ID,B0,B1,B2,B3>&) const;
template <bool B0,bool B1,bool B2,bool B3> bool operator >= (const CStrictInteger<NOT_STRICT_ID,B0,B1,B2,B3>&) const;
template <bool B0,bool B1,bool B2,bool B3> bool operator == (const CStrictInteger<NOT_STRICT_ID,B0,B1,B2,B3>&) const;
template <bool B0,bool B1,bool B2,bool B3> bool operator != (const CStrictInteger<NOT_STRICT_ID,B0,B1,B2,B3>&) const;

(このPRに含めてしまうことにしました。)

PR のメリット

  • 通常の単体テストでは不可能な こう書いたらエラーになる のテストを書けるようになります。

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

  • ビルド時間が長くなります。

仕様・動作説明

  • 仕様・既存アプリの動作は変更しません。

テスト内容

このPRの目的は「ビルドエラーになるはずのコード」のビルドが通ってしまうことの検出です。
意図的にビルドが通るように修正するコミットを積んで検出が行われることを確認します。
確認ができたら、追加コミットをrevertして話を進める予定です。

以下の画像が確認結果です。
compiletest

PR の影響範囲

  • アプリ(=サクラエディタ)の機能には影響しません。

関連 issue, PR

#1290 CLayoutIntのテストを追加
#1268 CStrictIntegerと整数を比較するグローバル演算子の実装を修正する

参考資料

https://cmake.org/cmake/help/v3.12/command/try_compile.html
https://cmake.org/cmake/help/v3.12/command/configure_file.html
https://docs.microsoft.com/en-us/visualstudio/msbuild/target-element-msbuild
https://cmake.org/cmake/help/v3.12/command/configure_file.html
https://cmake.org/cmake/help/v3.12/command/function.html

@AppVeyorBot
Copy link

Build sakura 1.0.2803 failed (commit bf63cdc893 by @berryzplus)

@AppVeyorBot
Copy link

@berryzplus berryzplus marked this pull request as ready for review May 10, 2020 10:58
@AppVeyorBot
Copy link

@m-tmatma
Copy link
Member

  • ビルド時間が長くなります。

どれぐらいですか?

@m-tmatma
Copy link
Member

MinGW のビルドに失敗していますが、修正中ですか?

@berryzplus
Copy link
Contributor Author

•ビルド時間が長くなります。

どれぐらいですか?

CMakeがCXXのconfigureを終えるのに必要な時間と同じくらい、環境によります。
ワンテンポ遅れるくらいのイメージです。うちの環境だとで2~10秒くらい。

MinGW のビルドに失敗していますが、修正中ですか?

MinGW側は何も触っていません。

https://dev.azure.com/sakuraeditor/sakura/_build/results?buildId=1719&view=logs&j=b39c645f-42c7-549c-3e61-e6ffdeb72915&t=a0663d8e-ebdb-5bf8-e394-e40b45d0a26a&l=87

In file included from window/CEditWnd.h:58,
                 from CAutoReloadAgent.cpp:28:
./outline/CDlgFuncList.h:145:31: error: extended character 、 is not valid in an identifier
  145 | 2002.04.01 YAZAKI SetTreeTxt()、SetTreeTxtNest()は廃止。GetTreeTextNextはもともと使用されていなかった。
      |                               ^

コメントの中の日本語の読点「、」が「不正な文字」と言われとります。
ビルド環境のトラブルっぽいですね。

@berryzplus
Copy link
Contributor Author

何度か再実行したけどダメでした。
cloneに失敗しているんだろうか・・・と思ったけどMSVC側はビルドできてるので謎。
というわけで、MinGW側にもコンパイルテストを含める変更をpushしてみます。

@AppVeyorBot
Copy link

@m-tmatma
Copy link
Member

何度か再実行したけどダメでした。
cloneに失敗しているんだろうか・・・と思ったけどMSVC側はビルドできてるので謎。
というわけで、MinGW側にもコンパイルテストを含める変更をpushしてみます。

master で失敗しているので #1298 に登録しました。

@berryzplus berryzplus force-pushed the feature/add_compiletest_of_clayoutint branch 2 times, most recently from e9dacf3 to 6294d1b Compare May 11, 2020 08:30
@berryzplus berryzplus force-pushed the feature/add_compiletest_of_clayoutint branch from 6294d1b to 113a704 Compare May 11, 2020 08:38
@AppVeyorBot
Copy link

Build sakura 1.0.2813 failed (commit bcae454f65 by @berryzplus)

@sakura-editor sakura-editor deleted a comment from AppVeyorBot May 11, 2020
@sakura-editor sakura-editor deleted a comment from AppVeyorBot May 11, 2020
@AppVeyorBot
Copy link

@berryzplus
Copy link
Contributor Author

MinGWビルドの問題解消後にrebaseしたので再レビューをお願いします。

Build sakura 1.0.2813 failed (commit bcae454 by @berryzplus)
  ↓
Build sakura 1.0.2814 completed (commit 57679b2 by @berryzplus)

@@ -0,0 +1,12 @@
<Project ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

質問。

この targets ファイルってどうやって作りましたか? 手書き?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

質問。
この targets ファイルってどうやって作りましたか? 手書き?

それは企業秘密なんですが・・・(なんでやねん!w

大枠は googletest.targets のコピーで、手書きです。
googletest.targets 自体は NuGet版GoogleTest をベースに作っています。

*.targets 自体は *.vcxproj*.csproj と同じ MsBuild に食わせるプロジェクト構成のXMLです。
質問で訊きたいことが把握できていませんが、それ専用のツールは知らないです。

compile_test( clayoutint_test.cpp.in "a >= b;" "CLayoutInt と CLogicIntを比較することはできない" clayoutint_can_not_be_compare_with_clogicint_greater_or_equal )
compile_test( clayoutint_test.cpp.in "a == b;" "CLayoutInt と CLogicIntを比較することはできない" clayoutint_can_not_be_compare_with_clogicint_equal )
compile_test( clayoutint_test.cpp.in "a != b;" "CLayoutInt と CLogicIntを比較することはできない" clayoutint_can_not_be_compare_with_clogicint_not_equal )
compile_test( clayoutint_test.cpp.in "int c = a;" "CLayoutInt は キャストなしでint型に代入することはできない" clayoutint_can_not_be_assigned_to_int )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

メモ

https://ci.appveyor.com/project/sakuraeditor/sakura/builds/32788007/job/x15n7byopmkgcqhd?fullLog=true

  -- Checking clayoutint_can_not_be_additive_assigned_from_clogicint
  -- Checking clayoutint_can_not_be_additive_assigned_from_clogicint - done
  -- Checking clayoutint_can_not_be_subtractive_assigned_from_clogicint
  -- Checking clayoutint_can_not_be_subtractive_assigned_from_clogicint - done
  -- Checking clayoutint_can_not_be_added_by_clogicint
  -- Checking clayoutint_can_not_be_added_by_clogicint - done
  -- Checking clayoutint_can_not_be_subtracted_by_clogicint
  -- Checking clayoutint_can_not_be_subtracted_by_clogicint - done
  -- Checking clayoutint_can_not_be_assigned_from_clogicint
  -- Checking clayoutint_can_not_be_assigned_from_clogicint - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_less_than
  -- Checking clayoutint_can_not_be_compare_with_clogicint_less_than - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_less_or_equal
  -- Checking clayoutint_can_not_be_compare_with_clogicint_less_or_equal - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_greater_than
  -- Checking clayoutint_can_not_be_compare_with_clogicint_greater_than - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_greater_or_equal
  -- Checking clayoutint_can_not_be_compare_with_clogicint_greater_or_equal - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_equal
  -- Checking clayoutint_can_not_be_compare_with_clogicint_equal - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_not_equal
  -- Checking clayoutint_can_not_be_compare_with_clogicint_not_equal - done
  -- Checking clayoutint_can_not_be_assigned_to_int
  -- Checking clayoutint_can_not_be_assigned_to_int - done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

メモ

berryzplus@113a704

https://ci.appveyor.com/project/sakuraeditor/sakura/builds/32786592/job/47k77o6abcf7a4dq?fullLog=true

  -- Checking clayoutint_can_not_be_additive_assigned_from_clogicint
  -- Checking clayoutint_can_not_be_additive_assigned_from_clogicint - done
  -- Checking clayoutint_can_not_be_subtractive_assigned_from_clogicint
  -- Checking clayoutint_can_not_be_subtractive_assigned_from_clogicint - done
  -- Checking clayoutint_can_not_be_added_by_clogicint
  -- Checking clayoutint_can_not_be_added_by_clogicint - done
  -- Checking clayoutint_can_not_be_subtracted_by_clogicint
  -- Checking clayoutint_can_not_be_subtracted_by_clogicint - done
  -- Checking clayoutint_can_not_be_assigned_from_clogicint
  -- Checking clayoutint_can_not_be_assigned_from_clogicint - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_less_than
  -- Checking clayoutint_can_not_be_compare_with_clogicint_less_than - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_less_or_equal
  -- Checking clayoutint_can_not_be_compare_with_clogicint_less_or_equal - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_greater_than
  -- Checking clayoutint_can_not_be_compare_with_clogicint_greater_than - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_greater_or_equal
  -- Checking clayoutint_can_not_be_compare_with_clogicint_greater_or_equal - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_equal
  -- Checking clayoutint_can_not_be_compare_with_clogicint_equal - done
  -- Checking clayoutint_can_not_be_compare_with_clogicint_not_equal
  -- Checking clayoutint_can_not_be_compare_with_clogicint_not_equal - done
  -- Checking clayoutint_can_not_be_assigned_to_int
  CMake Error at CMakeLists.txt:29 (message):
    Checking clayoutint_can_not_be_assigned_to_int - Failed.
  Call Stack (most recent call first):
    CMakeLists.txt:45 (compile_test)

@berryzplus
Copy link
Contributor Author

レビューありがとうございます。
マージしちゃいます。

@berryzplus berryzplus merged commit 7ac8117 into sakura-editor:master May 16, 2020
@berryzplus berryzplus deleted the feature/add_compiletest_of_clayoutint branch May 16, 2020 05:16
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020
…piletest_of_clayoutint

コンパイルテストを導入する
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants