Skip to content

tool: Add is_like_clang_cl() getter #1357

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants