-
Notifications
You must be signed in to change notification settings - Fork 165
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
COMエラー情報クラスを追加する #1533
COMエラー情報クラスを追加する #1533
Conversation
✅ Build sakura 1.0.3432 completed (commit ef2e634393 by @sanomari) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いいですね。ほぼ同等の修正を試みたことがあります。趣旨として異論はありません。
細かいところでいくつか修正提案を書き込んでみたので対応検討をお願いします。
* 2. 生成したらAddRef()し、不要になったらRelease()してください。 | ||
*/ | ||
template<class TargetInterface, class DeleteorType = std::default_delete<TargetInterface>, std::enable_if_t<std::is_base_of_v<IUnknown, TargetInterface>, std::nullptr_t> = nullptr> | ||
class TComImpl : public TargetInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これ、初の hpp ファイルですね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
これ、初の hpp ファイルですね。
初の hpp ファイルだと何かあるんですか?
指摘の意図が分からなかったです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
エンコードチェックするpythonスクリプトの件を気にしました。
} | ||
catch (const _com_error& ce) { | ||
// 投げられた例外メッセージを取得できること | ||
ASSERT_STREQ(message, (const wchar_t*)ce.Description()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このPRのメリットは、この1行に集約されますね。。。
f95c990
to
1bf1b63
Compare
sakura_core/basis/CErrorInfo.cpp
Outdated
* コンストラクタ | ||
*/ | ||
CErrorInfo::CErrorInfo( | ||
const std::wstring_view& source, //!< [in] ソース |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string_view の引数は値で渡すのが一般的です。携帯電話から打ってるので詳しくは書けないんですが、規格化作業のうちから値渡し前提で設計されたクラスだったはずです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こんな記事見つけました。
https://yohhoy.hatenadiary.jp/entry/20171113/p1
✅ Build sakura 1.0.3435 completed (commit 802091bdba by @sanomari) |
1bf1b63
to
3c1fb37
Compare
Kudos, SonarCloud Quality Gate passed! |
✅ Build sakura 1.0.3436 completed (commit 00bda0d50f by @sanomari) |
|
||
if (nRefCount == 0) { | ||
const DeleterType deletor; | ||
deletor(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
気づくのが遅くて申し訳ないのですが、typoしてますね。そのままにしておいても問題ないと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここ、少し直したんですが気付きませんでした。
レビューありがとうございます。 何かあれば気軽に修正提案あげてみてください。 |
PR の目的
Windowsのエラー情報を提供するCOMエラー情報クラスを導入して、例外を使ったコードを書けるようにします。
カテゴリ
PR の背景
サクラエディタは、ほとんど例外を利用していません。
プログラム中でエラーが発生した場合、その場でメッセージボックスを使ってエラー情報を表示しています。
エラー時にメッセージボックスを使うコードは自動テストができないコードになるのでテストを書く文化に移行させようとすると消えていく運命になるのが普通です。しかし、サクラエディタではメッセージボックス表示関数をカスタマイズすることにより、メッセージボックスを表示しても処理が止まらないように細工して自動テストとの共存を実現しています。
最近のC++では処理とエラー表示は意図的に分離します。
エラーを検出する機能は処理内容の一部ですが、どのように表示するかは処理と関係ないからです。
例外を使えば処理とエラー表示を分離することができます。
ただ、C++の例外クラス std::exception に指定できるメッセージは
const char*
なので、ちょっと難しいです。Windows向けC++コードで
const char*
に格納できるものは「現在のコードページ」でエンコードされた文字列です。システム言語が日本語な場合、Shift-JISでエンコードした文字列を格納することになります。
これは、Shift-JISで扱えない文字はメッセージに含められないという意味です。
また、Shift-JISに特化して変換するコードを書いても、英語や中国語でおかしくなる危険があります。
このPRでは、Unicodeのままのメッセージを投げられる方法を模索します。
PR のメリット
例外を利用したコードを書けるようになります。
PR のデメリット (トレードオフとかあれば)
とくに思いつきません。
仕様・動作説明
例外を使えば、メッセージを表示するのは「必要な場合だけ」とすることができます。
PR の影響範囲
メッセージ例外を投げられるようにするためのクラスを追加するだけなので、既存コードへの影響はありません。
テスト内容
同時に追加する単体テストケースで十分な検証ができると思います。
関連 issue, PR
参考資料