-
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
正規表現キーワードの一致判定が0文字マッチをマッチとみなさないように変更する #1030
正規表現キーワードの一致判定が0文字マッチをマッチとみなさないように変更する #1030
Conversation
✅ Build sakura 1.0.2217 completed (commit 00c7596250 by @berryzplus) |
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.
CRegexKeyword::RegexKeyCompile
でコンパイルする際に 0 文字マッチと判別出来るなら、RK_NOMATCH
を REGEX_INFO::nFlag
に指定するというのはどうでしょうか?
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で行わなくても良いんじゃないかな…と思いました。
コンパイルする段階では何文字マッチになるか分からないと思ってます。 例
どこ直したらいいか、リファクタしてみて始めて分かった感じです。 |
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.
動作確認した感じ問題無いと思います。
レビューありがとうございます。 |
変更内容に問題はないと考えていますが、懸念が一つ。 正規表現キーワードの0文字マッチ時の挙動を応用したウラ技がある、という自慢げなカキコミをいつかどこかでみかけたような気がしています。ざっと探してみた限り発見できなかったのでマージしてしまいましたが、もしかしたら「動作を変えられたら困る!」って人がいるかも知れません。 まぁ、そん時はそん時でちゃんと解決策を考えることにします。 |
…zero_width_match 正規表現キーワードの一致判定が0文字マッチをマッチとみなさないように変更する
PR の目的
正規表現キーワードのパターン一致条件を追加することにより、
0文字マッチをマッチと認識することによる不具合を解消します。
改修に当たっては、コード理解のために「未使用プリプロセッサの除去」と「部分式をローカル変数に切り出して意味を予測しやすい名前を付ける」の対応を行っています。レビューでも必要な情報だと思うので、あえて変更を残したままPRします。
カテゴリ
PR の背景
改修根拠については #1020 (comment) を見てください。
そもそも、仕様的に、正規表現キーワードって0文字マッチさせたらダメじゃね? がこのPRの趣旨です。iniの読込みや設定画面にチェックを入れる話にするとややこしくなるので、設定に関しては従来通りとし、マッチ検出時に「マッチ幅が0文字だったらそのマッチを無視する」という変更を入れます。
PR のメリット
PR のデメリット (トレードオフとかあれば)
PR の影響範囲
関連チケット
#1027
#1028
参考資料