-
Notifications
You must be signed in to change notification settings - Fork 520
[FEAT] Removed C code already ported to Rust #1738
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes C code implementations that have been replaced with Rust equivalents, representing a migration towards Rust-based implementations for various utility functions and features.
- Removed conditional compilation flags (
DISABLE_RUSTmacros) and associated C fallback implementations - Cleaned up build system files to eliminate Rust-optional compilation paths
- Streamlined codebase by removing deprecated C function implementations that were ported to Rust
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib_ccx/utility.h | Removed conditional compilation guards around Rust function declarations |
| src/lib_ccx/utility.c | Removed C implementations of utility functions already ported to Rust |
| src/lib_ccx/params.c | Removed large sections of C parameter parsing functions replaced by Rust |
| src/lib_ccx/lib_ccx.h | Removed DISABLE_RUST conditional compilation for function declarations |
| src/lib_ccx/hardsubx_*.c | Removed hardsubx C implementations replaced by Rust |
| src/lib_ccx/ccx_encoders_common.c | Removed C encoding functions and kept only Rust function calls |
| src/lib_ccx/ccx_decoders_*.c | Removed C decoder implementations in favor of Rust equivalents |
| src/lib_ccx/cc_bitstream.c | Removed C bitstream handling functions replaced by Rust |
| Build files | Removed WITHOUT_RUST and DISABLE_RUST options from build systems |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
I'm not sure if this is the same failing option test that you mentioned during the meeting, but it seems that this specific failure is related to this PR. I was able to reproduce it locally. |
|
Okay, I'll look into it. |
|
Removing all reference to the old C code would assume that the new RUST code is 100% functional. |
That's what the tests are for.
Personally I'm OK with this. We have previous versions if someone needs them. The problem with keeping the C code around is that we can't have clean Rust at the same time. At some point we need to start removing the legacy code, which will force everyone who wants to be up to date use the code we actually want to maintain and use. |
Fair enough, it would be helpful to create a tag for the last known version to contain C code before this PR is committed so if need be one can check out that tag and build using C. For example, currently RUST doesn't work for OCR but C does. I had to build ccextractor by disabling RUST to get it to work, and still take advantage of the bug fixes since the last release in 2021. |
Then we need to fix that. If we keep the old code around then there's no motivation to really solve the problem. But the migration to rust is precisely to address all these things - have a more stable code base that has no segfaults and is better written in general. |
|
Hi, I've fixed that one failing regression test in the PR for both linux and windows. Thanks to @hrideshmg for pointing this out. |
prateekmedia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 3f44115...:
All tests passing on the master branch were passed completely. Check the result page for more info. |
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit 3f44115...:
Congratulations: Merging this PR would fix the following tests:
All tests passing on the master branch were passed completely. Check the result page for more info. |
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
This PR removes the C code which were ported to Rust already, as discussed in zulip.
This PR also re-makes the entire Mac
build.command, as before this, we were always compiling on Mac with DISABLE_RUST on.