Conversation
waynevanson
force-pushed
the
refactor_allow_to_expect
branch
from
9d9ed51 to
092baf1
Compare
|
|
||
| # Append a Last. | ||
| entries.append(f'#[allow(dead_code)] Last = {first + len(entries)}') | ||
| entries.append(f'#[expect(dead_code)] Last = {first + len(entries)}') |
There was a problem hiding this comment.
I don't understand the context regarding this codegen, so I cannot don't know the impacts of changing this.
Should this file be omitted or should it stay?
There was a problem hiding this comment.
This need to be allowed and not expected because this code and other previously applied changes in this file are not dead code.
I'm not sure if removing this lint will break compatibility for something moving forward.
Should I remove this lint exception or leave it as allow?
There was a problem hiding this comment.
See #40882 why we shouldn't have expect. Instead, try to determine the configuration in which it must be added. If that's not possible, we would need to keep this allow
There was a problem hiding this comment.
Thank you. It's used in the output of the servo codegen crate, and I can see Last enum member being consumed in many places.
Therefore I've removed allow(dead_code)
waynevanson
force-pushed
the
refactor_allow_to_expect
branch
from
b8b2bf5 to
1c1132e
Compare
waynevanson
requested review from
gterzian,
jschwe,
nicoburns and
sagudev
as code owners
servo-highfive
added
the
S-awaiting-review
waynevanson
force-pushed
the
refactor_allow_to_expect
branch
2 times, most recently
from
498f7d6 to
7ba979d
Compare
TimvdLippe
left a comment
TimvdLippe
left a comment
There was a problem hiding this comment.
Some questions regarding these expect(dead_code). The rest LGTM. Let's wait for the other folks to respond. If you don't want to wait for that, you can extract the other ones out in a separate PR and we can merge these already, while we figure out what to do with the remaining ones.
Thanks for continuing this cleanup work!
| #[allow(clippy::too_many_arguments)] | ||
| #[allow(dead_code)] | ||
| #[expect(dead_code)] | ||
| pub(crate) fn new( |
There was a problem hiding this comment.
We can remove this method altogether, as the callers should use new_with_proto instead
There was a problem hiding this comment.
Great! This has now been removed.
components/servo/tests/common/mod.rs
Outdated
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| #[expect(dead_code)] |
There was a problem hiding this comment.
These are used in some of the test suites, but not the others: 9c86ef4
I think if you run ./mach test-unit -d now, they will no longer compile. Instead, please add a comment why this should be allowed
There was a problem hiding this comment.
You're right, this command does not compile in these tests.
I've reverted it to use allow, but now added comments and the suite now at least compiles.
I'm unable to get this command to pass on NixOS as it can't find shared files in the fonts. test-tidy and build have no issues.
I've got direnv with .envrc containing use nix which should've worked out of the box.
| } | ||
|
|
||
| #[allow(unused)] | ||
| #[expect(unused)] |
There was a problem hiding this comment.
Based on https://github.com/servo/servo/actions/runs/20637858589/job/59265304092 these are indeed unused.
ports/servoshell/egl/app.rs
Outdated
|
|
||
| #[cfg(feature = "webxr")] | ||
| #[cfg_attr(target_env = "ohos", allow(dead_code))] | ||
| #[cfg_attr(target_env = "ohos", expect(dead_code))] |
There was a problem hiding this comment.
I think this whole line can be removed, since it's only available when built with #[cfg(feature = "webxr")] anyways.
There was a problem hiding this comment.
I've removed this and ran ./mach check --feature webxr and ./mach check and found it compiles.
| } | ||
|
|
||
| #[allow(unused)] | ||
| #[expect(unused)] |
There was a problem hiding this comment.
I think this is used in
servo/ports/servoshell/egl/ohos/mod.rs
Lines 241 to 254 in b974a66
There was a problem hiding this comment.
I don't see these mentioned in https://github.com/servo/servo/actions/runs/20637858589/job/59265304092 so I think they are unused indeed. Not sure what the missing usage here is then, given that App::new is clearly used. Maybe one of the other methods is unused?
|
|
||
| #[expect(clippy::large_enum_variant)] | ||
| #[cfg_attr(any(target_os = "android", target_env = "ohos"), allow(dead_code))] | ||
| #[cfg_attr(any(target_os = "android", target_env = "ohos"), expect(dead_code))] |
There was a problem hiding this comment.
I see usages for these in
servo/ports/servoshell/egl/ohos/mod.rs
Lines 199 to 206 in b974a66
servo/ports/servoshell/egl/android/mod.rs
Lines 178 to 187 in a41ed4c
@mrobinson Same question here
There was a problem hiding this comment.
It's unclear to me why these aren't mentioned, since clearly https://github.com/servo/servo/actions/runs/20637858589/job/59265304092 doesn't complain about it. So I guess it's dead code after all.
servo-highfive
added
S-needs-code-changes
Signed-off-by: Wayne Van Son <[email protected]>
Signed-off-by: Wayne Van Son <[email protected]>
…per_case_globals)` Signed-off-by: Wayne Van Son <[email protected]>
Signed-off-by: Wayne Van Son <[email protected]>
Signed-off-by: Wayne Van Son <[email protected]>
Signed-off-by: Wayne Van Son <[email protected]>
Signed-off-by: Wayne Van Son <[email protected]>
Signed-off-by: Wayne Van Son <[email protected]>
Signed-off-by: Wayne Van Son <[email protected]>
Signed-off-by: Wayne Van Son <[email protected]>
Signed-off-by: Wayne Van Son <[email protected]>
Signed-off-by: Wayne Van Son <[email protected]>
Signed-off-by: Wayne Van Son <[email protected]>
waynevanson
force-pushed
the
refactor_allow_to_expect
branch
from
7ba979d to
96e0d8b
Compare
Signed-off-by: Wayne Van Son <[email protected]>
waynevanson
left a comment
waynevanson
left a comment
There was a problem hiding this comment.
I appreciate the feedback, thank you!
|
I won't split this PR up, happy to wait since I didn't structure commits nicely as it's my first PR to servo. |
servo-highfive
added
S-awaiting-review
TimvdLippe
added
T-android
github-actions
bot
removed
T-android
|
🔨 Triggering try run (#20637858589) for Android, OpenHarmony |
Bencher Report
|
✨ Try run (#20637858589) succeeded. |
TimvdLippe
left a comment
TimvdLippe
left a comment
There was a problem hiding this comment.
I inspected the relevant build logs and don't see any issues introduced by these fixes. So let's merge it and in case we have missed anything after all, we need to update our CI configuration to catch that.
| } | ||
|
|
||
| #[allow(unused)] | ||
| #[expect(unused)] |
There was a problem hiding this comment.
Based on https://github.com/servo/servo/actions/runs/20637858589/job/59265304092 these are indeed unused.
| } | ||
|
|
||
| #[allow(unused)] | ||
| #[expect(unused)] |
There was a problem hiding this comment.
I don't see these mentioned in https://github.com/servo/servo/actions/runs/20637858589/job/59265304092 so I think they are unused indeed. Not sure what the missing usage here is then, given that App::new is clearly used. Maybe one of the other methods is unused?
|
|
||
| #[expect(clippy::large_enum_variant)] | ||
| #[cfg_attr(any(target_os = "android", target_env = "ohos"), allow(dead_code))] | ||
| #[cfg_attr(any(target_os = "android", target_env = "ohos"), expect(dead_code))] |
There was a problem hiding this comment.
It's unclear to me why these aren't mentioned, since clearly https://github.com/servo/servo/actions/runs/20637858589/job/59265304092 doesn't complain about it. So I guess it's dead code after all.
servo-highfive
removed
the
S-awaiting-review
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
|
Thank you for the reviews @TimvdLippe, I appreciate it. I'll get the next set of changes ready for review. |
4 participants
Replace
allowwithexpectlints forunused,unsafe_code,dead_code, andnon_upper_case_globals.Testing: So far just check it compiled on
x86_64-linuxon NixOS. Need to use the modulesystem.fontconfig.enable = true;I think in my NixOS config.Part of: #40383
Searching
allow\(.*\)for.rsfiles shows the following. for (total_results:total_files) went from707:386to675:368, a reduction of32:18.How many files is too many files per PR? I feel like the 20-30 I have is too big.