Skip to content

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
@ban-nobuhiro
Copy link
Contributor

コード変更が私の担当でしたが、 当PR への反映を長らく放置してしまい申し訳ありません。
@kuron99 レビューをよろしくお願いします。

@ban-nobuhiro
Copy link
Contributor

PR owner を reviewer に足すことはできないようなのでそのままにしてあります。

@kuron99
Copy link
Contributor Author

kuron99 commented May 12, 2025

@ban-nobuhiro 変更ありがとうございます。レビューしました。問題ありません。

自分がPR ownerだとapprovalも足せないようです。

@ban-nobuhiro
Copy link
Contributor

では、前半分と後半分を合わせて approve としました。

PR対象の ブランチからさらにブランチする PR を作るということを前にされたことがあるので、
そういう方法でやるとよかったのかもしれません。

@kuron99
Copy link
Contributor Author

kuron99 commented May 12, 2025

PRのオーナーシップを共有したり渡したりというのが本来の想定された使い方ではないんですね。

@kuron99
Copy link
Contributor Author

kuron99 commented May 16, 2025

PRオーナーが作業する必要があるんですね。時間がたったのでrebaseしてからmergeしようと思います。

@kuron99 kuron99 force-pushed the doc_invalid_range branch from b3c147f to baf941f Compare May 16, 2025 07:06
@kuron99 kuron99 merged commit 615af9b into master May 16, 2025
4 checks passed
@kuron99 kuron99 deleted the doc_invalid_range branch May 16, 2025 07:09
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