Skip to content

Do not feature-gate generated dead-code #251

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

Closed
ia0 opened this issue Jan 8, 2021 · 7 comments
Closed

Do not feature-gate generated dead-code #251

ia0 opened this issue Jan 8, 2021 · 7 comments
Assignees

Comments

@ia0
Copy link
Member

ia0 commented Jan 8, 2021

Generated code (like derives) don't generate warnings when unused. They also don't end up in the final binary when unused (library size and compilation time shouldn't matter and are negligible). So there seems to be no reason to feature-gate them, which currently decreases readability and increases maintenance.

Current example snippet:

#[cfg_attr(feature = "derive_debug", derive(Debug))]

Proposed example snippet:

#[derive(Debug)]
@ia0
Copy link
Member Author

ia0 commented Jan 8, 2021

FYI: If we are afraid of mistakenly using Result::unwrap which requires Debug, we could enable the unwrap_in_result clippy lint.

@jmichelp
Copy link
Collaborator

jmichelp commented Jan 8, 2021

I remember an issue in Tock where this actually had impact on the produced binary (perf + size). But maybe this has either been fixed in the toolchain or I'm mixing things up.

Regarding clippy, I still have to check why clippy workflow doesn't work properly in the Github workflows. It doesn't treat warnings as errors despite the corresponding flag being set.

@ia0
Copy link
Member Author

ia0 commented Jan 8, 2021

I remember an issue in Tock where this actually had impact on the produced binary (perf + size). But maybe this has either been fixed in the toolchain or I'm mixing things up.

Ok, that's what @kaczmarczyck said too. Maybe there used to be some bug then (maybe because our target architecture wasn't well supported at that time). But now it looks like it's fixed (at least for our crypto library and persistent storage too).

Regarding clippy, I still have to check why clippy workflow doesn't work properly in the Github workflows. It doesn't treat warnings as errors despite the corresponding flag being set.

Ah ok too bad. So we might want to wait until that's fixed then. Also note that the lint I mentioned is "allow" by default, so we would need to explicitly make it "warn" (or more).

@jmichelp
Copy link
Collaborator

jmichelp commented Jan 8, 2021

Found the issue I mentioned: tock/libtock-rs#84

@kaczmarczyck
Copy link
Collaborator

@jmichelp this is good to close, if you don't mind not having unwrap_in_result.

@ia0
Copy link
Member Author

ia0 commented Feb 2, 2021

Or alternatively (or in addition) a workflow to print the binary size diff as a message in PR discussions.

@kaczmarczyck
Copy link
Collaborator

The workflow exists, and clippy::unwrap_in_result is too strict IMO.

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

No branches or pull requests

3 participants