-
Notifications
You must be signed in to change notification settings - Fork 171
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
HeaderMake出力メッセージの英語化 #57
Conversation
HeaderMake/HeaderMake.cpp
Outdated
@@ -58,6 +58,8 @@ | |||
#endif // __MINGW32__ | |||
|
|||
#include <stdio.h> | |||
// #include <stdlib.h> |
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.
これ何ですか?
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.
消し忘れでした。消します!
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.
git commit --amend で対応しちゃいましたが消しました
"\n" | ||
" Mode\n" | ||
" define : Output .h file as #define list\n" | ||
" enum : Output .h file as enum list\n" |
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.
<enum名>を指定すればそれが列挙型の名前になる
の部分がなくなっています
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.
EnumName (Optional) : Enum name (when enum mode only)
という箇条書きを追加してます。それだと伝わらないですかね?
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.
了解です。
const char* in_file = NULL; | ||
const char* out_file = NULL; | ||
const char* mode_name = NULL; | ||
const char* enum_name = ""; |
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.
"英語化" には関係ないですが、英語化以外の修正もされていのでコメントしますが、
ここの const はおかしくないですか? 途中で書き換えています。
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.
C++ 的にはおかしくはないです。
const char* in_file = NULL;
は in_file
が参照する char 領域を書き換え不可にするだけで、in_file のポインタを書き換え不可にする意味ではないです。
ポインタ自体を書き換えできないようにする場合は const char* const in_file = NULL;
みたいな感じに書きます。
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.
そうでした。失礼しました。
|
||
|
||
|
||
//���� | ||
//処理 | ||
|
||
if(mode==MODE_DEFINE){ | ||
fprintf(out, | ||
"#ifndef SAKURA_HEADERMAKE_98B26AB2_D5C9_4884_8D15_D1F3A2936253_H_\n" |
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.
これも英語化には関係ないですが、
SAKURA_HEADERMAKE_98B26AB2_D5C9_4884_8D15_D1F3A2936253_H_ 等の値はマクロや
const の変数などでまとめて定義したほうがいいです。
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.
その通りではあるのですが、使用箇所が少ないのでそのままでも良いかな…と。
ちなみにこのインクルードガードの追加実装は 2009 年に入ったもののようですが、
今となっては #pragma once
に対応していないコンパイラってあまり無いと思うので #pragma once
にしたい気持ちが強いです。このあたりはやるとしたら別件として処理したいです。
あと、#56 で指摘されているように HeaderMake 自体がいらなくなる可能性もなくはないので、細部にこだわって作りこんでも仕方ないのでは、という気持ちもあります。ただ現状文字化けが AppVeyor 上で発生するのは気持ち悪いので、このPRは取り急ぎの対処という感じです。
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.
変更するのは、メッセージだけに限定するべきだと思います。メッセージの英語化に必要ない修正も含まれています。
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.
コメントの繋がりがおかしいので、補足すると
メッセージの英語化に関係ない修正もあったので、関係ない部分にコメントしましたが、
本来的には、英語化だけにした方がいいと思ってます。
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名がダメだったかもしれません。
本PRの意図は「メッセージを見やすくする」ことで、それ以外の変更は加えていません。
少し大きく変更しているように見えるのは、開始メッセージと終了メッセージで処理を挟んだところで、そのあたりを綺麗に書くために少し関数を分割しました。
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.
開始メッセージと終了メッセージを入れないと、AppVeyor 上のログを見てもどこからどこまでが HeaderMake のログだか分からなかった、という事情があります。
・ついでにソースコードのエンコーディングを UTF-8(BOM有り) に変更 ・ついでにHeaderMakeの開始/終了メッセージを挿入 ・ついでにUsageを少し丁寧にした
af1e91b
to
30f6b05
Compare
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.
いちおう動いていて、英語化する目的が達成されているからとりあえずはOKなんではないかと思っています。
細かいことを言い出すと、そもそもこのプログラムはcl.exeに依存するものではなくなってるので、修正するならコメントも直したほうが良いはずです。
ここでプリプロセッサを実行している。
sakura/HeaderMake/HeaderMake.cpp
Lines 210 to 211 in 6f23813
sprintf_s(in_file2,_countof(in_file2),PREPROCESSOR,in_file); | |
FILE* in=_popen(in_file2,"rt"); if(!in ){ printf("Error: 入力ファイルを開けません\n"); return 1; } //※プリプロセス済みストリーム |
今回のPR目的は、文字化けする日本語メッセージを英語化することなので、その目的が達成されることをもってよしとするのは(妥協ですが)ありだと思っています。
Review結果は一応approveにしておきますので、このままマージするかどうかは @m-tmatma さんの判断にお任せします。
approve ありがとうございます。
補足しておくと cl.exe 依存は残ってます。 |
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.
動作も問題ないを確認しました。
@m-tmatma さん、丸投げしてすみません。対応ありがとうございます。 @kobake さん、言いたかったのは「cl.exeに依存→cl.exe(or gcc.exe)に依存」ということでした。 sakura/HeaderMake/HeaderMake.cpp Lines 72 to 82 in 3e71335
gccでビルドするとgcc依存になると思います。 何気にgit cloneたまにコケるの謎が未解決だったりもしますが・・・ |
なるほど、そこの変更は把握していませんでした。ご説明ありがとうございます。 |
…lish HeaderMake出力メッセージの英語化
HeaderMakeの出力メッセージが環境問わず見れるように(文字化けせず見れるように)英語出力に変更。