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

resolve z_check ambiguity #492

Closed
milyin opened this issue Jul 2, 2024 · 5 comments
Closed

resolve z_check ambiguity #492

milyin opened this issue Jul 2, 2024 · 5 comments
Labels
api fix Problem in the API release Part of the next release

Comments

@milyin
Copy link
Contributor

milyin commented Jul 2, 2024

Describe the release item

The z_xxx_check is pair function to z_xxx_null. It should return "true" if object is in a "null" state.
The z_xxx_null creates "stub" owned object which is

  1. safe to be forgotten without explicit dropping. This is the state which object is set after z_drop operation
  2. may be unusable, e.g. z_loan on it may crash

The problem is that for some objects this null/stub state is valid. E.g. empty buffer or default encoding.

The proposal is to make this interface change:

  • use word "empty" instead of "null". The "null" implies that object is invalid, which is not always the case. Pair of functions "z_xxx_empty" / "z_xxx_is_empty" will set/get information if object is in empty state
  • make "z_check" return false only on objects which are invalid. E.g. z_check of empty session will return false, but z_check on empty buffer or default encoding will return true.

Also it makes sense to get rid of active usage of "z_check" in examples - in most cases error code from constructor is enough

@milyin milyin added release Part of the next release api fix Problem in the API labels Jul 2, 2024
@DenisBiryukov91
Copy link
Contributor

for non-blocking channels it is necessary to call z_check on objects returned by try_recv()

@milyin
Copy link
Contributor Author

milyin commented Aug 4, 2024

for non-blocking channels it is necessary to call z_check on objects returned by try_recv()

Ok, I see the problem. Separating z_check to z_is_empty/z_check may provoke more problems due to their misusage. I think now that it's better to keep only one z_check function. It should return true when and only when the object is in the same state, as it would have been created by z_xxx_null. It doesn't matter if the object is actually usable or not.

But the problem remains. The z_loan is unsafe operation now: there is no null check in it to make code faster. But z_check is not a complementary operation for z_loan: it returns false on empty objects which actually can be loaned. So we don't have a way to check if object can be loaned or not.

So I have another proposal which doesn't require API change right now. Let's make z_loan safe: it should return NULL when loaning null owned object. In most cases it shouldn't affect performance. For rare cases when it does we can later provide z_loan_unsafe which doesn't do this check.
@yellowhatter , @DenisBiryukov91 , @sashacmc what do you think?

@milyin
Copy link
Contributor Author

milyin commented Aug 5, 2024

After discussion with @DenisBiryukov91 the proposal is to make z_check usable on loaned objects. This means that even for objects in gravestone state the z_loan is allowed.
Though I still not completely sure what's the role of z_check in this case. Should it return true or false on slice with null data pointer for example? Should it always return true for Encoding which is always valid?
And we still lack for the way for checking if the owned object is in gravestone state or not.

Another solution is to declare that z_check is a function complementary to z_null and it returns true when and only when the object is in the "gravestone" state (which is result of z_null or z_drop or failed constructor).
There is no way to check if the object loanable or not, this depends on object. This is a hole in the API, but it can be resolved later if necessary

@milyin
Copy link
Contributor Author

milyin commented Aug 7, 2024

After discussion with @DenisBiryukov91 and discussion between @DenisBiryukov91 and @Mallets as far as I understand the decision to make z_check and z_null undocumented was made. Please correct me if I'm wrong.

Though this raises question about our guarantees about double drop safety and about gravestone state of objects in case of failed construction. z_null is used to set owned object to gravestone state. z_check tests if object is in this state. Without these functions there is no way to check for these guarantees.

The double-drop guarantee and the gurantee that failed constructor leaves owned object in safe to drop state are necessary for C++ wrapper. So we have to provide this state and test for this state anyway.

So I see no reason to hide this state from user and I think that z_null and z_check should remain

@milyin
Copy link
Contributor Author

milyin commented Aug 8, 2024

After discussion with @Mallets the final decision is to rename them to _z_check and _z_null to keep them out of official API but still use in tests for check for our double drop guarantees

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api fix Problem in the API release Part of the next release
Projects
Status: Done
Development

No branches or pull requests

2 participants