static castやnullopt比較をやめ、value関数などを使うようにする #118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oyaki122
requested changes
Apr 20, 2022
There was a problem hiding this comment.
PRありがとうございます
とても見やすくなったと思います
一つ確認なのですが、synthesis_engine.cpp 207行目などの、同じ変数でhas_value()に変更している箇所があるにもかかわらず、std::nulloptとの比較から変更していない部分については、どのように可読性が悪いと思いましたか？
確かに変数名が長いものに関しては==で区切りがあったほうが可読性が高いと思います
ただこの箇所などについては個人的にあまり可読性が悪いと感じなかったため、問題なければ統一したほうがいいのではないでしょうか
お手数をおかけしますがよろしくお願いします
|
C++ の optional に有効値/無効値が入っていることを判定するやり方について調べてみたのですが、 std::optional<SomeType> opt_var = get_opt_sometype();
// 有効値が入っていることの確認
if (opt_var) {
// 中身を取り出す
SomeType value = opt_var.value();
...
}
// 無効値が入っていることの確認
if (!opt_var) {
...
}というような方法が最も簡潔そうだったので、提案してみます。 参考： https://cpprefjp.github.io/reference/optional/optional.html |
おっと、これは単純に見落としていたようです。
確かに、こちらの方がきれいそうです...! |
y-chan
force-pushed
the
feature/remove-static-cast
branch
from
07a0524 to
07b1caa
Compare
y-chan
changed the title
Yosshi999
requested changes
Apr 21, 2022
| base_index += matched_text->size(); | ||
| stack = ""; | ||
| matched_text = std::nullopt; | ||
| } else { | ||
| throw std::runtime_error("unknown text in accent phrase: " + stack); | ||
| } | ||
| if (outer_loop > LOOP_LIMIT) throw std::runtime_error("detect infinity loop!"); | ||
| } | ||
| if (accent_index == std::nullopt) throw std::runtime_error("accent not found in accent phrase: " + phrase); |
There was a problem hiding this comment.
Suggested change
| if (accent_index == std::nullopt) throw std::runtime_error("accent not found in accent phrase: " + phrase); | |
| if (!accent_index) throw std::runtime_error("accent not found in accent phrase: " + phrase); |
There was a problem hiding this comment.
あと83行目もありますが、そこに関しては「中身があるならthrow」というぱっと見非自明なif文なのでどちらが可読性が良いのかは不明です
There was a problem hiding this comment.
83行目のことも考慮して現在のコードになっています。
統一したほうが可読性が高いと思うので、こちらもそのままにすべきと思いましたが、どうでしょうか?
Yosshi999
approved these changes
May 15, 2022
| base_index += matched_text->size(); | ||
| stack = ""; | ||
| matched_text = std::nullopt; | ||
| } else { | ||
| throw std::runtime_error("unknown text in accent phrase: " + stack); | ||
| } | ||
| if (outer_loop > LOOP_LIMIT) throw std::runtime_error("detect infinity loop!"); | ||
| } | ||
| if (accent_index == std::nullopt) throw std::runtime_error("accent not found in accent phrase: " + phrase); |
|
@Oyaki122 申し訳ないです、change request があるのを見落としていました。再レビューをお願いしたいです……！ |
PickledChair
pushed a commit
that referenced
this pull request
Jul 24, 2022
* デフォルト引数はC言語では使えないのでC++のみに有効になるように変更した (#122) 本当はデフォルト引数を消したかったが、使ってる人がいる可能性があるためC++で使ってる場合はそのままにするように修正した * initializeで全モデルを読み込まなくても良いようにした (#124) * load_model関数を切り出す * load_modelとis_model_loadedを足した * 使われてないエラーメッセージを消去 * Update core/src/core.cpp Co-authored-by: qwerty2501 <939468+qwerty2501@users.noreply.github.com> * Update core/src/core.cpp Co-authored-by: qwerty2501 <939468+qwerty2501@users.noreply.github.com> * Update core/src/core.cpp Co-authored-by: qwerty2501 <939468+qwerty2501@users.noreply.github.com> * pythonのラッパーの型を変更 * load_all_models追加 * return true Co-authored-by: qwerty2501 <939468+qwerty2501@users.noreply.github.com> * workflow_dispatchでもビルド開始可能に (#127) * ビルド時にREADMEファイルを追加する (#131) * append unit testing framework (#121) * Catch2をプロジェクト導入して参考となる1ケースを追加した * リリース用ビルドにはテストビルドは不要のため無効化する変数を追加した * テストコードの短縮化を行った * READMEにテスト実行方法を追加 * テストをビルドするかどうかのフラグをデフォルトOFFにした * テストをビルドするかどうかのフラグを変更したことによりREADMEを修正 * MSVCに対して足りないオプションを追加 - utf8文字列でコンパイルするように指定 - C++20を明示的に指定 - __cplusplusの値を利用中のC++のバージョンに合わせるように指定(ないとC++98相当になるとか) * Configがリリースの場合のみに最適化オプションを指定するように修正 * オプションの指定を短くまとめた * coreの標準ライブラリもバージョンアップした * compile optionsの修正 * Catch2にもCXX_STANDARD 20を追加 * Windows環境でビルドできるように設定を修正 #121 (comment) * format 効いてしまっていたところを修正 * static castやnullopt比較をやめ、value関数などを使うようにする (#118) * use value and has value * remove has_value * coreのビルド時にバージョン情報がちゃんと 入るようにする * hotfix イントネーションの推論をCPUで行うように (#146) * python example for 0.12, update FFI (#138) * .gitignore open_jtalk_dic in example (#148) * .gitignore open_jtalk_dic in example * modify: example/python/.gitignore * C++サンプルコードへのリンクを追加 (#142) * C++サンプルコードへのリンクを追加 * #readme * Update README.md * コード署名できるようにする (#164) * コード署名 * build_util * artifact/ * a * remove * inputのbooleanは文字列として渡ってくるので判定を修正 (#166) * inputのbooleanは文字列として渡ってくるので判定を修正 * 修正もれ Co-authored-by: Hiroshiba <hihokaruta@gmail.com> Co-authored-by: Yuto Ashida <y-chan@y-chan.dev> Co-authored-by: Yosshi999 <Yosshi999@users.noreply.github.com> Co-authored-by: nebocco <73807432+nebocco@users.noreply.github.com>
y-chan
added a commit
to SHAREVOX/sharevox_core
that referenced
this pull request
Sep 1, 2022
* use value and has value * remove has_value
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
内容
最初コードを書いていたときに、
has_value/value関数を知らず、後から知って、こちらの方がコードが綺麗になることがわかったので、リファクタリングとして変更しました。ただし、一部
std::nulloptとの比較の方が直観的で可読性が高い場所があったので、そこは残してあります。