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

correct behavior with invalid scan range #47

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kuron99
Copy link
Contributor

@kuron99 kuron99 commented Feb 25, 2025

不正なscan rangeを渡された際の挙動についてAPI文書を更新し、必要であればコード変更も行います。

@kuron99 kuron99 requested a review from ban-nobuhiro February 25, 2025 09:18
include/kvs.h Outdated
* std::string_view{nullptr, non-zero-value}.
* @return Status::ERR_BAD_USAGE The input argument or the range given by the arguments are invalid (see note above.)
* This includes the case where one of the endpoints uses scan_endpoint::INF and the corresponding key is not null
* (i.e. different from default constructed std::string_view.)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここは少し強い条件を書きすぎだったかもしれません。scan_endpoint::INFの際にkeyに何を渡していいかということはまだ合意していなかった気がするのですが、std::string_view{nullptr, <non-zero>}をチェックして弾くくらいなら std::string_view{} を強制してもいいのではないかと思ったところです。もう少し弱い条件もありかもしれないのでご意見ください。

Copy link
Contributor

Choose a reason for hiding this comment

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

std::string_view{nullptr, non-zero-value} のくだりは、むしろ INF でない場合のことを記載していたのだと思います。
ただ、もともとそのように string_view を構築した場合には undefined behavior なので (cppreference) チェックして ERR_BAD_USAGE を返すこと (UB からの脱出) を明言するのは無理があるので削除したほうが良いと思います。

INF のときの key の扱いについては、これがどう記述されていたのかを把握していなかったので、曖昧であれば正したほうが良いと考えていましたが、もともと INF の場合には key は無視すると明示的に書かれていたんですね。それならば元のままでも別に構わないとは思いますが、空文字列に限ったほうがプログラミングミスに早く気づけて親切だ、等の意向であればそれも構いません。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

std::string_view{nullptr, non-zero-value} のくだりは、むしろ INF でない場合のことを記載していたのだと思います。

空のstring_viewを渡す妥当なケースはINFしかないと思ったのですが、確かにもとの文はそれに限定しているわけでもないですね。

ただ、もともとそのように string_view を構築した場合には undefined behavior なので (cppreference) チェックして ERR_BAD_USAGE を返すこと (UB からの脱出) を明言するのは無理があるので削除したほうが良いと思います。

同意です。そういうstd::string_viewを作った時点でUBであれば、その先の仕様を決めても意味がないですね。

INF のときの key の扱いについては、これがどう記述されていたのかを把握していなかったので、曖昧であれば正したほうが良いと考えていましたが、もともと INF の場合には key は無視すると明示的に書かれていたんですね。それならば元のままでも別に構わないとは思います

ではINFについては以前の仕様のまま(無視する)ということにしましょう。今回はレンジの話が主なので。

Copy link
Contributor

@ban-nobuhiro ban-nobuhiro left a comment

Choose a reason for hiding this comment

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

細かい点のコメントを入れました。
では具体的にどうするのか、を書かないのもどうかと思い、書いては見たのですが、そちらは参考程度でお願いします。

include/kvs.h Outdated
* and otherwise an error (Status::ERR_BAD_USAGE) will be returned. A range is invalid if any of the following
* conditions are satisfied.
* - `r_key` < `l_key` and neither `l_end` nor `r_end` is scan_endpoint::INF
* - `l_key` equals to `r_key` and either `l_end` or `r_end` is scan_endpoint::EXCLUSIVE
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. l_key == r_key でいずれかの end指定が INF である場合は valid になることもあります (例えば両 ley が空文字列で r_end が INF の場合)。
    上の行のように 「and neither l_end nor r_end is scan_endpoint::INF」 を足せば正しくなると思います。
  2. INF の場合は key には関心がないので、 key 条件を書いてから INF か? と書くのではなく、 INF か? を書いてから key 条件を書いたほうが良いと思います。

例えばこうなるかと思います。

  • neither l_end nor r_end is scan_endpoint::INF and r_key < l_key
  • neither l_end nor r_end is scan_endpoint::INF and l_key equals to r_key and either l_end or r_end is scan_endpoint::EXCLUSIVE

2文目が長すぎるので、以下のような書き方も考えましたが、かえってわかりづらいかもしれません。

  • neither l_end nor r_end is scan_endpoint::INF AND

    • r_key < l_key

    OR

    • l_key equals to r_key AND either l_end or r_end is scan_endpoint::EXCLUSIVE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

l_key == r_key でいずれかの end指定が INF である場合は valid になることもあります

INF指定の場合はkeyをヌルに強制させようと思っていたのでこのケースは入れていませんでした。INF指定時にkeyは自由(無視される)になるので書く必要がありますね。INFを先にすべきというのもそのとおりと思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

少し冗長ですが、前者の書き方(neitherを2回書く)にしようかと思います。その方がany of the following conditions are satisfiedの指す following conditions が明確だと思うので。

include/kvs.h Outdated
* conditions are satisfied.
* - `r_key` < `l_key` and neither `l_end` nor `r_end` is scan_endpoint::INF
* - `l_key` equals to `r_key` and either `l_end` or `r_end` is scan_endpoint::EXCLUSIVE
k - `r_key` is empty string and `r_end` is scan_endpoint::EXCLUSIVE
Copy link
Contributor

Choose a reason for hiding this comment

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

typo かと思いますが行頭アスタリスクが k になっています。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

typoですね。直します。

@kuron99 kuron99 requested a review from ban-nobuhiro February 26, 2025 01:11
@kuron99 kuron99 changed the title docs: description on invalid scan range correct behavior with invalid scan range Feb 26, 2025
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.

2 participants