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

Remove cfg-if and noop-proc-macro crates #19

Merged
merged 2 commits into from
Mar 10, 2024

Conversation

FreezyLemon
Copy link
Contributor

This isn't very important, but it feels like these crates don't add much value in this project. I guess they were used in rav1e and just kept as-is?

I think that cfg-if is unnecessary for simple cfg attributes like this, and the noop_proc_macro is used so rarely that it barely reduces the amount of code.

@codecov-commenter
Copy link

codecov-commenter commented Dec 28, 2023

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 66.52%. Comparing base (31ef767) to head (cbee199).

Files Patch % Lines
src/plane.rs 0.00% 2 Missing ⚠️
src/frame.rs 0.00% 1 Missing ⚠️
src/pixel.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #19   +/-   ##
=======================================
  Coverage   66.52%   66.52%           
=======================================
  Files           4        4           
  Lines         923      923           
=======================================
  Hits          614      614           
  Misses        309      309           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Luni-4 Luni-4 requested a review from lu-zero January 17, 2024 09:48
@lu-zero
Copy link
Member

lu-zero commented Jan 17, 2024

We'd need to add a wasm target and serialize/deserialize tests to make sure we do not break stuff.

If it makes a v_frame consumer lighter it could land, but I'd rather not risk breaking stuff we hardly test.

@FreezyLemon
Copy link
Contributor Author

I think these macros are just helpers to simplify #[cfg] attributes and the serde/wasm_bindgen derives. I might be missing something, but I'm pretty sure these don't influence the functionality at all.

cargo expand shows that the resulting code is effectively the same even for different feature and target (wasm) compilations.

@lu-zero
Copy link
Member

lu-zero commented Jan 17, 2024

I mean in the future, not having to sprinkle cfg_attr across the codebase is nice since you cannot miss one when you add code.

@FreezyLemon
Copy link
Contributor Author

Ah, I see what you mean. I think the risk of that is relatively low, but if it's a point of concern then feel free to close this.

@Luni-4
Copy link
Member

Luni-4 commented Jan 29, 2024

Can we add a wasm target job on Continuous Integration so that we are sure not to break anything?

@FreezyLemon
Copy link
Contributor Author

Can we add a wasm target job on Continuous Integration so that we are sure not to break anything?

I think expanding the CI tests makes sense. Can you open an issue for it? It's a bit out of scope for this PR.

@Luni-4
Copy link
Member

Luni-4 commented Feb 29, 2024

@FreezyLemon

Can this one be unblocked due to ac33688?

@FreezyLemon
Copy link
Contributor Author

I need to at least rebase this because the wasm feature doesn't exist anymore. It's probably fine to unblock this, but we should also ask @lu-zero because it was their suggestion.

@FreezyLemon FreezyLemon force-pushed the fewer-dependencies branch 2 times, most recently from c5a6bc7 to a7e641c Compare March 5, 2024 18:39
@FreezyLemon
Copy link
Contributor Author

Small hiccup while rebasing, should be good now.

Copy link
Collaborator

@shssoichiro shssoichiro left a comment

Choose a reason for hiding this comment

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

I think this looks good. Would like approval from @lu-zero on this one though due to prior conversation

Copy link
Member

@lu-zero lu-zero left a comment

Choose a reason for hiding this comment

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

Thank you for following through :)

@lu-zero lu-zero merged commit f0c70fd into rust-av:main Mar 10, 2024
5 of 6 checks passed
@FreezyLemon FreezyLemon deleted the fewer-dependencies branch March 10, 2024 19:49
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.

5 participants