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

CLayoutIntのテストを追加 #1290

Merged

Conversation

berryzplus
Copy link
Contributor

@berryzplus berryzplus commented May 9, 2020

PR の目的

メタクラス CLayoutInt の単体テストを追加します。

カテゴリ

  • その他の問題

PR の背景

#1268 CStrictIntegerと整数を比較するグローバル演算子の実装を修正する

#1268 (comment)

作っていただいたテストを、別pr投げてもらえますか?

#1268 で試したのは比較演算子だけだったんですが、せっかくなので他の演算子のテストも実装してみました。

結論だけ言うと CLayoutInt って 単なる int です。

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

本来の機能に対する automake 系のテストもそのうち書きますが、とりあえず挙動確認のたたきを作るのがこのPRの目的になります。

PR のメリット

  • CLayoutInt に単体テストが追加される。
    • 今後加えた変更がクラスの挙動に影響を与えるかどうかの判断がラクになる。
    • (副次効果として) どんな機能があるのか把握しやすくなる。

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

  • 単体テストは「ビルドされたコード」をテストするものである都合、CLayoutInt の本来の目玉機能である「許可されない変換を行おうとした場合にビルドエラーになる」のテストは実施できない。
  • 単体テストはできるだけシンプルに、間違いの起こりにくいように書いているが、これが絶対に正しいという保証はない。

仕様・動作説明

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

テスト内容

  • 追加する対象の単体テストを実行し、passすることを確認しました。

PR の影響範囲

  • 今後の CLayoutInt の機能追加・仕様変更に影響を与える可能性があります。
  • アプリ(=サクラエディタ)の機能に影響しません。

関連 issue, PR

#1268 CStrictIntegerと整数を比較するグローバル演算子の実装を修正する

参考資料

https://docs.microsoft.com/en-us/cpp/cpp/cpp-built-in-operators-precedence-and-associativity
http://stlalv.la.coocan.jp/Operator.html

@AppVeyorBot
Copy link

ついでに剰余算の説明を少し拡充。
@AppVeyorBot
Copy link

// コメントアウトの組み合わせは未実装。
CLayoutInt a( 2 ), b( 3 ), c( 6 );

//EXPECT_TRUE( c == a * b );
Copy link
Member

Choose a reason for hiding this comment

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

実装されていないとわかったものに関しては、別チケット作っておいたほうがいいと思います。
すぐに対処する必要はないとは思いますが、記録だけお願いします。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1295 立てました。

@berryzplus
Copy link
Contributor Author

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

@berryzplus berryzplus merged commit 5112f0e into sakura-editor:master May 10, 2020
@berryzplus berryzplus deleted the feature/add_test_of_clayoutint branch May 10, 2020 05:29
HoppingTappy pushed a commit to HoppingTappy/sakura that referenced this pull request Jun 16, 2020
…t_of_clayoutint

CLayoutIntのテストを追加
@beru beru added the UnitTest label Jul 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants