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

Add: 辞書コンパイルに-qオプションを追加 #8

Merged
merged 11 commits into from
Jul 6, 2023

Conversation

sevenc-nanashi
Copy link
Member

@sevenc-nanashi sevenc-nanashi commented Jul 5, 2023

内容

mecab-dict-indexに-q/--quietオプションを追加します。

関連 Issue

スクリーンショット・動画など

(なし)

その他

C++は初心者なので結構変な書き方してると思います。

フォーマッタは設定が見当たらなかったのでかけてません。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

readingとかが勝手に出力されるのは確かに避けたいですね!!

エラーメッセージ(cerr)はさすがに出たほうが良いかもと感じました。
正常時には出ないはずでしょうし。

@sevenc-nanashi
Copy link
Member Author

確かに。
エラーは出すようにしてみます。

(本来はResult型みたいなの作ってそれを返すべきな気がしますが、それをやれるほどの知識がない)

@Hiroshiba
Copy link
Member

改修ありです!あとは

エラーは出すようにしてみます。

こちらですかね!

@qryxip qryxip self-requested a review July 6, 2023 11:35
@sevenc-nanashi
Copy link
Member Author

エラーは出るようにしました。

❯ cmake -B build && cmake --build build && ./build/simple_example 
-- Configuring done
-- Generating done
-- Build files have been written to: /home/sevenc7c/voicevox/open_jtalk/__gi_test/build
Consolidate compiler generated dependencies of target simple_example
[100%] Built target simple_example
mecab-dict-index -d ../../core/crates/test_util/data/open_jtalk_dic_utf_8-1.11 -u ./user.dic -f utf-8 -t utf-8 main.cpp -q 
/home/sevenc7c/voicevox/open_jtalk/src/mecab/src/dictionary.cpp(345) [n == 5] format error: #include "mecab.h"
Segmentation fault
[ble: exit 139]
Details
#include "mecab.h"
#include <pthread.h>
#include <stdio.h>
#include <sys/wait.h>
#include <unistd.h>

int main(int argc, char *argv[]) {
  const char *mdi_argv[] = {
      "mecab-dict-index",
      "-d",
      "../../core/crates/test_util/data/open_jtalk_dic_utf_8-1.11",
      "-u",
      "./user.dic",
      "-f",
      "utf-8",
      "-t",
      "utf-8",
      "main.cpp",
      "-q"
  };
  for (auto arg: mdi_argv) {
    printf("%s ", arg);
  }
  printf("\n");

  int ret = mecab_dict_index(sizeof(mdi_argv) / sizeof(mdi_argv[0]),
                             const_cast<char **>(mdi_argv));
  printf("ret = %d\n", ret);
  return 0;
}

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!!

C++の実装の良し悪しはちょっと分かりませんが、いい感じに馴染んだんじゃないかなと思います!
(もし何か問題がありそうだったらちょっとこちらでrevertけさせてもらうかもしれません。)

@Hiroshiba Hiroshiba merged commit 7e7a946 into VOICEVOX:1.11 Jul 6, 2023
5 checks passed
@qryxip
Copy link
Member

qryxip commented Jul 8, 2023

(本来はResult型みたいなの作ってそれを返すべきな気がしますが、それをやれるほどの知識がない)

C++内かつC++23であればstd::expectedでしょうか?

(C ABIだとtagged unionでいけると思うのですが、それをやっているのは聞いたことは無いですね...)

@sevenc-nanashi
Copy link
Member Author

なるほど。
どちらにせよ伝播させるいい感じの方法が思いつかないので、まぁエラーが起きたらやるみたいな感じでいいような気がします

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants