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

byte_stream changes that add error exceptions have changed previous behavior #668

Closed
bretzlaff opened this issue Feb 8, 2023 · 3 comments
Assignees
Labels

Comments

@bretzlaff
Copy link

byte_stream.h changes in SHA 6a8cdf6892367deed444eadbed0288138ca5e301 change the API and break the expected behavior.

Before this change, byte_stream_writer::write (it's many implementations) would return a bool true/false if there was room in the stream for the data. The current implementation now generates an ASSERT when the available space check fails and would never return a false value from the function call.

This implementation basically changes the behavior from an interface that returned true/false and allowed the caller to handle the issue themselves to now forcing an ASSERT to occur.

I would suggest a few alternatives to preserve the original behavior and implement your newer functionality.

  • Move the ETL_ASSERT_FAIL into the write_unchecked function since the standard write functions are being checked already
  • create an additional function, for example write_with_exception(...) to implement the new functionality
  • provide either an optional parameter and/or implement a compile time switch that implements the exception behavior
@bretzlaff
Copy link
Author

Also, the published API specifies that true/false will be returned from these functions.

@jwellbelove
Copy link
Contributor

Yes, I think the addition of the error asserts was a mistake.
I think this is also true for etl::bit_stream_reader and etl::bit_stream_writer.

@jwellbelove jwellbelove self-assigned this Feb 20, 2023
@jwellbelove
Copy link
Contributor

Fixed 20.35.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

2 participants