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

Add copysign #207

Merged
merged 7 commits into from
May 3, 2022
Merged

Add copysign #207

merged 7 commits into from
May 3, 2022

Conversation

XAMPPRocky
Copy link
Contributor

Resolves #152
Closes #155

@XAMPPRocky
Copy link
Contributor Author

@cuviper Would you be able to help out with why the CI is failing? I don't really understand what the macro is generating or how it works, and the diff looks identical to PRs that have succeeded to me.

@cuviper
Copy link
Member

cuviper commented Mar 31, 2021

The standard library didn't get copysign until 1.35. In CI for earlier versions, you're hitting #![deny(unconditional_recursion)], as the attempted macro-forwarding actually ends up calling the trait method again, instead of the newer inherent method. You'll need to add a build.rs check for the method, and then cfg the forwarding based on that, leaving it to the default impl otherwise.

@XAMPPRocky XAMPPRocky force-pushed the copysign branch 3 times, most recently from 770deed to e6e9769 Compare April 22, 2021 06:54
@XAMPPRocky
Copy link
Contributor Author

You'll need to add a build.rs check for the method, and then cfg the forwarding based on that, leaving it to the default impl otherwise.

I think I've implemented that, but it's still failing the tests, and I still can't really tell what I need to do to fix it.

src/float.rs Outdated Show resolved Hide resolved
@XAMPPRocky
Copy link
Contributor Author

The workflow needs to be approved :)

@XAMPPRocky
Copy link
Contributor Author

@tspiteri Thanks for that, it seems like it's passing now.

@XAMPPRocky
Copy link
Contributor Author

@cuviper Friendly review ping 🙂

src/float.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated
@@ -23,6 +23,7 @@ libm = { version = "0.2.0", optional = true }
default = ["std"]
std = []
i128 = []
copysign = []
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need a feature for this? I'd rather leave it to autocfg, unless there's a strong reason to expose that condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because autocfg is not something that can be used on custom targets (related rust-lang/cargo#7501). I would go as far as saying that autocfg is an anti-pattern for supporting custom targets currently, the way probing capabilities currently works is very incompatible if your project requires being built in a specialised way. One example a project that needs num-traits and a custom target is rust-gpu.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, even as the autocfg author I am frustrated by that situation, but I haven't found time to design/push for tool changes that would alleviate it. I think having something like TARGET_RUSTFLAGS from cargo set for build scripts would solve a lot of issues. Might you be interested in pursuing this, since it impacts targets you work with?

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, autocfg uses CARGO_ENCODED_RUSTFLAGS now, which should be more reliable.

If you're not even happy with that, then I'd prefer a plain version check rather than a feature for every little implementation detail. ("i128" is a little different because it actually adds to the API.)

build.rs Outdated Show resolved Hide resolved
src/float.rs Outdated Show resolved Hide resolved
src/float.rs Outdated Show resolved Hide resolved
@XAMPPRocky
Copy link
Contributor Author

@cuviper I believe I've fixed the issues, but you'll need to approve the workflow first to check.

ctrlcctrlv pushed a commit to MFEK/num-traits.rlib that referenced this pull request Jan 13, 2022
Signed-off-by: Fredrick Brennan <copypaste@kittens.ph>
@ctrlcctrlv
Copy link
Contributor

(Merged into MFEK@ce9a7df due to #189 (comment).)

@cuviper
Copy link
Member

cuviper commented May 2, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented May 3, 2022

@bors bors bot merged commit edb4821 into rust-num:master May 3, 2022
ctrlcctrlv pushed a commit to MFEK/num-traits.rlib that referenced this pull request Mar 28, 2023
Signed-off-by: Fredrick Brennan <copypaste@kittens.ph>
ctrlcctrlv pushed a commit to MFEK/num-traits.rlib that referenced this pull request Mar 28, 2023
Signed-off-by: Fredrick Brennan <copypaste@kittens.ph>
ctrlcctrlv pushed a commit to MFEK/num-traits.rlib that referenced this pull request Mar 28, 2023
Signed-off-by: Fredrick Brennan <copypaste@kittens.ph>
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.

Float trait doesn't require copysign function
4 participants