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.

Sign up for GitHub

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

Jump to bottom

Return status code instead of bool in IBlockEncoder & IBlockDecoder #747

Merged
merged 1 commit into from
Jul 26, 2024
Merged

Return status code instead of bool in IBlockEncoder & IBlockDecoder #747

merged 1 commit into from
Jul 26, 2024

Conversation

runei
Copy link

@runei runei commented Jul 10, 2024

Fixes #738

@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot added the work in progress Pull request is still in progress and changing label Jul 10, 2024
@runei runei changed the title Return status code instead of bool in IBlockEncoder & IBlockDecoder #738 Return status code instead of bool in IBlockEncoder & IBlockDecoder Jul 10, 2024
@gavv gavv added the contribution A pull-request by someone else except maintainers label Jul 11, 2024
@gavv

This comment was marked as outdated.

Sign in to view
@gavv

This comment was marked as outdated.

Sign in to view
@runei runei marked this pull request as ready for review July 15, 2024 23:06
@github-actions github-actions bot added ready for review Pull request can be reviewed and removed work in progress Pull request is still in progress and changing labels Jul 15, 2024
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot added needs rebase Pull request has conflicts and should be rebased and removed needs rebase Pull request has conflicts and should be rebased labels Jul 17, 2024
@gavv gavv added review in progress Pull request is being reviewed and removed ready for review Pull request can be reviewed labels Jul 19, 2024
@gavv gavv self-requested a review July 19, 2024 11:00
@github-actions github-actions bot added the ready for review Pull request can be reviewed label Jul 19, 2024
@gavv gavv removed the ready for review Pull request can be reviewed label Jul 19, 2024
gavv
gavv reviewed Jul 20, 2024
View reviewed changes
Copy link
Member

@gavv gavv left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! Here are a few comments.

src/internal_modules/roc_fec/target_openfec/roc_fec/openfec_decoder.h Outdated Show resolved Hide resolved
src/internal_modules/roc_fec/target_openfec/roc_fec/openfec_encoder.h Outdated Show resolved Hide resolved
src/internal_modules/roc_fec/iblock_decoder.h Outdated Show resolved Hide resolved
src/internal_modules/roc_fec/iblock_encoder.h Outdated Show resolved Hide resolved
src/tests/roc_fec/test_block_encoder_decoder.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_fec/block_reader.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_fec/block_reader.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_fec/block_writer.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_fec/block_writer.cpp Outdated Show resolved Hide resolved
src/tests/roc_fec/test_block_writer_reader_errors.cpp Show resolved Hide resolved
@gavv gavv added needs revision Pull request should be revised by its author and removed review in progress Pull request is being reviewed labels Jul 20, 2024
@runei runei requested a review from gavv July 23, 2024 17:27
@github-actions github-actions bot added ready for review Pull request can be reviewed and removed needs revision Pull request should be revised by its author labels Jul 23, 2024
@gavv gavv added this to the next milestone Jul 25, 2024
@gavv
Copy link
Member

gavv commented Jul 25, 2024

Thank for update, LGTM!

Could you rebase (not merge) your branch on fresh develop? Currently it's not rebasable due to conflicts.

PS. Please don't resolve discussions by yourself, usually reviewer does it when they re-check code. Instead you can use a thumbs up or leave a comment that the issue is addressed (if you need to track discussions for yourself).

@gavv gavv added the needs rebase Pull request has conflicts and should be rebased label Jul 25, 2024
@github-actions github-actions bot removed the needs rebase Pull request has conflicts and should be rebased label Jul 25, 2024
@gavv gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels Jul 25, 2024
@runei
Copy link
Author

runei commented Jul 25, 2024

Thank for update, LGTM!

Could you rebase (not merge) your branch on fresh develop? Currently it's not rebasable due to conflicts.

PS. Please don't resolve discussions by yourself, usually reviewer does it when they re-check code. Instead you can use a thumbs up or leave a comment that the issue is addressed (if you need to track discussions for yourself).

Hi @gavv , the branch is rebased.

@gavv gavv added ready for review Pull request can be reviewed and removed needs revision Pull request should be revised by its author labels Jul 25, 2024
@gavv gavv merged commit 5cf7355 into roc-streaming:develop Jul 26, 2024
@gavv
Copy link
Member

gavv commented Jul 26, 2024

Thank you!

@github-actions github-actions bot removed the ready for review Pull request can be reviewed label Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavv gavv Awaiting requested review from gavv

Assignees
No one assigned
Labels
contribution A pull-request by someone else except maintainers
Projects
None yet
Milestone
next
Development

Successfully merging this pull request may close these issues.
2 participants
@runei @gavv