Fix #539 「Texのファイルで落ちます。」 #553
Conversation
* Fix: トピックの階層が連続しない場合に、トピックが抜けたり、未初期化変数にアクセスして落ちないように。 * Fix: バッファ長より長いトピックタイトルで落ちないように。 * Fix: ３桁以上になる連番で落ちないように。 * Chg: 連番なしのトピックがあるときでも階層が正しくなるように。
|
テストデータをこの PR に貼り付けていただけますか? |
|
例外データは issue #539 の KENCHjp さん投稿のものを使いました。 正常データは同じ issue の #539 (comment) で投稿したマクロで作成しました。マクロが出力した正常データの例を、コメントとして javascript ファイルに含めています。 修正した同マクロで出力した、完全にランダムな例外データをここにアップロードします。random_topic.tex.txt |
sakura_core/types/CType_Tex.cpp
Outdated
|
|
||
| // '\' の後ろから、'{' を目印にタグとタイトルを見つける。 | ||
| pTag = ++p; | ||
| p = (p = wmemchr(p, L'{', pLineEnd - p)) ? p : pLineEnd; |
There was a problem hiding this comment.
c#でよく見る書き方の移植版だと思えばそれほど複雑でもないような。
中括弧を探して見つかった位置を取得、見つからなければ行末位置を取得、というたけの処理なんで。
@beru さんあたりからツッコミ入るだろうと思って静観しておりました。
複雑さに関しては問題ないと思ってます。
たぶんこれ、バラすとかえって分かりづらくなる気がします。何がしたいか読み取りにくくなると思います。今だったら説明されれば分かるコードだと思うんです。
There was a problem hiding this comment.
wmemchr の戻り値に不満があるんです。NULL では使いにくい。STL ならこういう場合には end に相当する値を返してきます。
軽量言語なら || や or を使うところですし、gcc なら条件演算子の真の場合の値を省いた ?: を使って途中の代入が省けるので、まだ読みやすくなるんですが。
There was a problem hiding this comment.
こう書くのはどうでしょうか？
while(p<pLineEnd&&*p!='{')++p;There was a problem hiding this comment.
それは元の書き方です。手続きではなく、目的を書くために wmemchr を使いました。不本意な wmemchr の戻り値に加えて、|| 演算子が使えなかったのも誤算でしたが。
There was a problem hiding this comment.
元のコードが wcsstr を使っていたことでなんとなく <wchar.h> 縛りを課していましたが、異論百出で無理があったようです。STL の戻り値なら素直に書けるのですから、std::find で書き直しました。
There was a problem hiding this comment.
std::find は目的に合致していて良いですね。C++アレルギーをこじらせてしまっているせいか自分には思い付きませんでした。
sakura_core/types/CType_Tex.cpp
Outdated
|
|
||
| // トピック文字列を作成する(2)。タイトルをバッファに埋め込む。 | ||
| const ptrdiff_t copyLen = t_min(szTopic + _countof(szTopic) - 1 - pTopicEnd, pTitleEnd - pTitle); | ||
| pTopicEnd = wmemcpy(pTopicEnd, pTitle, copyLen) + copyLen; |
There was a problem hiding this comment.
https://msdn.microsoft.com/ja-jp/library/dswaw1wk.aspx
によると wmemcpy は dest の値 を返す、とあります。
単に pTopicEnd += copyLen;
とした方が簡潔になると思いますが、なぜこうしているのですか?
There was a problem hiding this comment.
↑ wmemcpy(pTopicEnd, pTitle, copyLen); とは別に分けて書くという意味です。
There was a problem hiding this comment.
書き込みと書き込みポインタの更新はアトミックな処理であって欲しいということです。せめて見た目上だけでも。
wmemcpy の戻り値に不満があります。書き込んだ後の位置であるか、書き込んだ長さを返してくれたら使いやすいんですが、dst そのものを返してきます。
There was a problem hiding this comment.
書き込みと書き込みポインタの更新はアトミックな処理であって欲しいということです。せめて見た目上だけでも。
このように書いても特に何か変わることはなく、
単に、wmemcpy の戻り値の仕様なんだっけ?
第一引数を返すのか、pTopicEnd を直接更新すればいいじゃん、ということになるだけで
特にメリットはないように思います。
関数が処理したサイズとかを利用するのなら意味あるのですが、
この場合は単に読みにくくなるだけだと思います。
There was a problem hiding this comment.
微妙ですが、ここは「記述を分ける」
に1票です。
書き込みと書き込みポインタの更新をアトミックにしたい意図はわかるんです。ioの場合はそちらが普通だとも思います。ただここはメモリ書き込みなので、Ioのように失敗の考慮が要らず、実際の処理はアトミックではないと思います。
既存コードはどうか？で行くと
行を分けてmemcpyの戻り値は捨ててるコードが多い気がします。
あと、個人的には分けた方が読みやすい気がします。
There was a problem hiding this comment.
不本意だという wmemcpy の戻り値を知らなければ理解できないうえに読みにくいとあれば、拘る理由はありませんでした。分けます。気づくのが遅れて煩わせてしまいました。:pensive:
sakura_core/types/CType_Tex.cpp
Outdated
| void CDocOutline::MakeTopicList_tex(CFuncInfoArr* pcFuncInfoArr) | ||
|
|
||
| /** アウトライン解析の補助クラス */ | ||
| template<int Size> |
There was a problem hiding this comment.
TagHierarchy の要素数? 何のサイズなのかが変数名から自明になるようにしてほしいです。
There was a problem hiding this comment.
MakeTagProcessor を使えば意識する必要はありませんし、コンストラクタを見ればわかりますので、わかってください、としか。重要ではないのでそっけない名前を付けています。
とは、一度書きかけましたが、思ったより Size - 1 が頻出するわけではなかったので、HierarchyMax 変数と統合して HierarchyCount としておきました。
There was a problem hiding this comment.
MakeTagProcessor を使えば意識する必要はありませんし、コンストラクタを見ればわかりますので、わかってください、としか。重要ではないのでそっけない名前を付けています。
コメントで説明するのでもいいのですが、
単体で見たときに、これ何? みたいなのは避けてほしいです。
There was a problem hiding this comment.
Size はそれのみで存在するわけではなく、コンストラクタのパラメータによって定義されるものですので、説明的な名前を付けることに意味がないと考えています。変数名を付ける者の意図も使途もなく、ただ定義によって決まるものなので、それ以外の意味がありません。
Size が使われる場所において読みやすくなるのはたしか。予め -1 した使いやすい HierarchyMax 変数を使うことで、Size の使用は控えていました。
There was a problem hiding this comment.
TagHierarchy のサイズだというのは決まっているのだから、何を言っているのかと、自分で読んで思いました。
やはり、重要ではない、無視してよい、使われないものに、長い名前は似つかわしくないということです。
ds14050
force-pushed
the
FixTeXOutline
branch
2 times, most recently
from
5dd8ed0
to
b66c740
Compare
ds14050
force-pushed
the
FixTeXOutline
branch
from
b66c740
to
4a7bb37
Compare
There was a problem hiding this comment.
#539 (comment) に貼られているテキストのアウトライン解析をすると落ちる不具合が解消されているので PR としては問題無いと思います。
欲を言うとユニットテストが追加されていると良いと思いますが、ブーメランになるので独り言にしておきます。。ﾌﾞﾂﾌﾞﾂ…。
* Fix: トピックの階層が連続しない場合に、トピックが抜けたり、未初期化変数にアクセスして落ちないように。 * Fix: バッファ長より長いトピックタイトルで落ちないように。 * Fix: ３桁以上になる連番で落ちないように。 * Chg: 連番なしのトピックがあるときでも階層が正しくなるように。
Fix #539 「Texのファイルで落ちます。」
詳細は Issue #539 を参照してください。