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

Refactor streams: rename is_* to wait_* for clarity #2069

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

falbrechtskirchinger
Copy link
Contributor

  • Replace is_readable() with wait_readable() and is_writable() with wait_writable() in the Stream interface.
  • Implement a new is_readable() function with semantics that more closely reflect its name. It returns immediately whether data is available for reading, without waiting.
  • Update call sites of is_writable(), removing redundant checks.

Note: The new is_readable() capability is essential for the WebSockets code, as there's currently no way to tell if read() would block. I have an alternative proposal to add has_buffered_data(), which is identical to is_readable() in this PR. This alternative is less invasive and requires no other code changes, while this PR also aims to enhance semantic clarity.

- Replace is_readable() with wait_readable() and is_writable() with
  wait_writable() in the Stream interface.
- Implement a new is_readable() function with semantics that more
  closely reflect its name. It returns immediately whether data is
  available for reading, without waiting.
- Update call sites of is_writable(), removing redundant checks.
@falbrechtskirchinger falbrechtskirchinger changed the title Refactor streams: rename is_\* to wait_\* for clarity Refactor streams: rename is_* to wait_* for clarity Feb 19, 2025
@yhirose
Copy link
Owner

yhirose commented Feb 20, 2025

@falbrechtskirchinger thanks for the improvement! is_readable() is no longer used, right? If so, could you please remove it?

@falbrechtskirchinger
Copy link
Contributor Author

@falbrechtskirchinger thanks for the improvement! is_readable() is no longer used, right? If so, could you please remove it?

Please read the note in the OP. is_readable() is critical for the PRs I'll submit later today. Once all three features are in, I'll add unit tests.

@yhirose
Copy link
Owner

yhirose commented Feb 20, 2025

Sorry that I forgot to pay attention to the note... The code looks good. Thanks!

@yhirose yhirose merged commit 550f728 into yhirose:master Feb 20, 2025
7 of 9 checks passed
@falbrechtskirchinger
Copy link
Contributor Author

Sorry that I forgot to pay attention to the note... The code looks good. Thanks!

No worries! 👍

@falbrechtskirchinger falbrechtskirchinger deleted the stream-api-prop1 branch February 20, 2025 18:07
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