Skip to content

check f16 and f128 in float_equality_without_abs #15054

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

usamoi
Copy link

@usamoi usamoi commented Jun 14, 2025

followup of rust-lang/rust#141874

changelog: check f16 and f128 in float_equality_without_abs

@rustbot
Copy link
Collaborator

rustbot commented Jun 14, 2025

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 14, 2025
&& let ExprKind::Path(ref epsilon_path) = rhs.kind
&& let Res::Def(DefKind::AssocConst, def_id) = cx.qpath_res(epsilon_path, rhs.hir_id)
&& ([sym::f32_epsilon, sym::f64_epsilon].into_iter().any(|sym| cx.tcx.is_diagnostic_item(sym, def_id)))
&& (sym_epsilon.into_iter().any(|sym| cx.tcx.is_diagnostic_item(sym, def_id)))
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: you can use get_diagnostic_item to avoid a bunch of query overhead, something like

Suggested change
&& (sym_epsilon.into_iter().any(|sym| cx.tcx.is_diagnostic_item(sym, def_id)))
&& matches!(cx.tcx.get_diagnostic_name(def_id), Some(sym::f16_epsilon | sym::f32_epsilon | sym::f64_epsilon | sym::f128_epsilon))

Copy link
Author

Choose a reason for hiding this comment

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

It's too long so rustfmt complains error[internal]: line formatted, but exceeded maximum width.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can split the line by hand and make rustfmt happy.

@y21
Copy link
Member

y21 commented Jun 14, 2025

Can you also add tests for this? It seems like there's also a f16_f128 FIXME in the code that could easily be resolved in the same PR here:

// FIXME(f16_f128): add tests for these types when abs is available

@usamoi
Copy link
Author

usamoi commented Jun 14, 2025

Can you also add tests for this?

Added.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants