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

tool: Add is_like_clang_cl() getter #1357

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

MarijnS95
Copy link
Contributor

In ring we need to check if the compiler is clang-cl to prevent overwriting it with clang (see all details in briansmith/ring#2215), but cc-rs does not currently expose this despite already tracking it. By adding this getter we can steer that logic to not overwrite the compiler when it is clang-cl.

Perhaps, since ToolFamily is pub (but not yet reexported from lib.rs), could we have a public Tool::family() getter to not have to extend these is_xxx() getters whenever a downstream crate wants to know something new?

In `ring` we need to check if the compiler is `clang-cl`
to prevent overwriting it with `clang` (see all details in
briansmith/ring#2215), but `cc-rs` does not
currently expose this despite already tracking it.  By adding this
getter we can steer that logic to not overwrite the compiler when it
is `clang-cl`.

Perhaps, since `ToolFamily` is `pub` (but not yet reexported from
`lib.rs`), could we have a public `Tool::family()` getter to not have
to extend these `is_xxx()` getters whenever a downstream crate wants to
know something new?
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks!

could we have a public Tool::family() getter

Yes that'd makes sense with non_exhaustive enum

@NobodyXu NobodyXu merged commit 29a92bd into rust-lang:main Jan 10, 2025
52 checks passed
@NobodyXu
Copy link
Collaborator

cc usually has a new release every Friday (and we already have one today).

If this is very important, then I can cut another release today/tomorrow

@MarijnS95
Copy link
Contributor Author

Yes that'd makes sense with non_exhaustive enum

Let's see if I can make a second PR.

If this is very important, then I can cut another release today/tomorrow

Unsure, let's see how quickly ring wants to accept this. I don't personally have a big hurry with this, we can just [patch] away :)

@MarijnS95 MarijnS95 deleted the is-like-clang-cl branch January 10, 2025 14:19
@MarijnS95
Copy link
Contributor Author

cc usually has a new release every Friday (and we already have one today).

It looks like it failed: #1355

Meaning you could perhaps trivially roll this in :)

@NobodyXu
Copy link
Collaborator

Thanks, I forgot to check my release PR 😂

@MarijnS95
Copy link
Contributor Author

MarijnS95 commented Jan 10, 2025

@NobodyXu perhaps we don't need to release with it if we release with #1358 instead :). Might as well remove is_like_clang_cl() entirely then :)

MarijnS95 added a commit to MarijnS95/cc-rs that referenced this pull request Jan 10, 2025
Instead of adding a new `is_like_xxx()` function every time new
information is needed, or new enumeration variants would be added,
provide the (now `non_exhaustive`) `enum` directly to callers to match
what they're interested in.  Deprecate the existing functions to point
users to the new pattern.

Also removes `is_like_clang_cl()` again from rust-lang#1357 since that did not
yet make it into a release, and would only cause unnecessary duplication
in our API patterns.
MarijnS95 added a commit to MarijnS95/cc-rs that referenced this pull request Jan 10, 2025
Instead of adding a new `is_like_xxx()` function every time new
information is needed, or new enumeration variants would be added,
provide the (now `non_exhaustive`) `enum` directly to callers to match
what they're interested in.

Also removes `is_like_clang_cl()` again from rust-lang#1357 since that did not
yet make it into a release, and would only cause unnecessary duplication
in our API patterns.
@github-actions github-actions bot mentioned this pull request Jan 11, 2025
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.

2 participants