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

More cfg alias targets + disabled related clippy lint #2236

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

Jan561
Copy link
Contributor

@Jan561 Jan561 commented Nov 30, 2023

Adds the remaining supported platforms as cfg alias.

This PR also disables this lint.

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

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

Thanks!

@SteveLauC SteveLauC added this pull request to the merge queue Nov 30, 2023
Merged via the queue into nix-rust:master with commit e13a182 Nov 30, 2023
34 checks passed
@Jan561 Jan561 deleted the cfg_alias branch November 30, 2023 11:17
@asomers
Copy link
Member

asomers commented Nov 30, 2023

Why is there a need for those aliases?

@SteveLauC
Copy link
Member

Why is there a need for those aliases?

Yeah, they are currently not in use, should I revert this PR?

I merged this PR because it is trivial and maybe there will be a use case for it in the future, when we don't want those target_os = "xxx" stuff and fully switch to aliases

@asomers
Copy link
Member

asomers commented Nov 30, 2023

Yes, I think we should revert it , just so we can reenable that lint. I've seen that lint fire correctly before.

@Jan561
Copy link
Contributor Author

Jan561 commented Nov 30, 2023

  1. Either we use aliases for all supported targets, or we don't use them at all. Something in between is the worst option. I don't want to double check every time if there is a alias configured for the target.

  2. Converts AddressFamily to struct with associated consts #2210 already uses these aliases, and having to configure these in a PR when needing them feels wrong.

  3. This lint is necessary if we want to use #[cfg(linux)] instead of #[cfg(target_os = "linux")]

@asomers
Copy link
Member

asomers commented Nov 30, 2023

I don't think we should use them at all, if they only cover single targets. Aliases like apple_family are useful because they combine multiple targets. But the linux alias only saves a few characters. And I want to restore that lint precisely because I've seen people accidentally do #[cfg(linux)] where they meant #[cfg(target_os = "linux")]

@SteveLauC
Copy link
Member

#2210 already uses these aliases, and having to configure these in a PR when needing them feels wrong.

This is my fault, I am sorry about it

@Jan561
Copy link
Contributor Author

Jan561 commented Nov 30, 2023

#2210 already uses these aliases, and having to configure these in a PR when needing them feels wrong.

This is my fault, I am sorry about it

All good, just need to tweak my codegen script a bit.

github-merge-queue bot pushed a commit that referenced this pull request Dec 1, 2023
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.

3 participants