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

Document Sum::sum returns additive identities for [] #136710

Merged
merged 1 commit into from
Feb 9, 2025

Conversation

JakenHerman
Copy link
Contributor

@JakenHerman JakenHerman commented Feb 7, 2025

Because the neutral element of <fNN as iter::Sum> was changed to neg_zero, the documentation needed to be updated, as it was reporting inadequate information about what should be expected from the return.

Relevant Commit: 4908188
Relevant Pull Request: #129321


The referenced commit causes unintended side effects on presentation layer applications like using Tera templates, for example. I'm not sure what the motivation was behind the original change, but it seems like more discussion should be put into this issue and potentially have that change reverted.

@rustbot
Copy link
Collaborator

rustbot commented Feb 7, 2025

r? @Noratrieb

rustbot has assigned @Noratrieb.
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 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 7, 2025
@JakenHerman JakenHerman changed the title Update rustdoc for iterator.rs Update rustdoc for iterator.rs Feb 7, 2025
@workingjubilee
Copy link
Member

@JakenHerman The reason is simple, as the negative zero is the true neutral element for floats: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9d21ada96adf24c4d3f9f2ef360c9630

fn main() {
    assert!(( 0.0f32 +  0.0).is_sign_positive());
    assert!((-0.0f32 +  0.0).is_sign_positive());
    assert!(( 0.0f32 + -0.0).is_sign_positive());
    assert!((-0.0f32 + -0.0).is_sign_negative());
}

@workingjubilee
Copy link
Member

You would not expect the following to change the sign of the zero, would you?

[-0.0f32].into_iter().sum()

@workingjubilee workingjubilee changed the title Update rustdoc for iterator.rs Document Sum::sum returns additive identities Feb 7, 2025
@workingjubilee
Copy link
Member

Because the neutral element of <fNN as iter::Sum> was changed to neg_zero, the documentation needed to be updated, as it was reporting incorrect information about what should be expected from the return.

And I must note, the documentation was not technically incorrect.

...You can say it was uselessly correct, of course.

@workingjubilee workingjubilee changed the title Document Sum::sum returns additive identities Document Sum::sum returns additive identities for [] Feb 7, 2025
@workingjubilee
Copy link
Member

I'm curious, can you describe the effects on the Tera applications?

@JakenHerman
Copy link
Contributor Author

JakenHerman commented Feb 7, 2025

I'm curious, can you describe the effects on the Tera applications?

@workingjubilee Sure, I'll give a simple example... Consider an HTML/PDF report that sums up financial transactions then displays the total value of the summed transactions. If there are no transactions, the report would show that you spent $-0.00, whereas it used to be $0.00.

That's a very simple example (and not the one I ran into), but it is the easiest I could come up with for illustrative purposes. I suppose the real solution on the template side would be to abs the value before presenting it, but it is slightly jarring to have numerous reports mysteriously and suddenly reporting negative 0 values in production.

I committed your suggestion, as I think you summed up the expected return value better than I did. Thanks for your input.

@workingjubilee
Copy link
Member

That is slightly unfortunate, yes. I guess that would have worked out fine if you just always had a positive value in the summation such that even if it reached zero, it would have been positive.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 7, 2025

📌 Commit 1093779 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2025
Because the neutral element of `<fNN as iter::Sum>` was changed to
`neg_zero`, the documentation needed to be updated, as it was reporting
inadequate information about what should be expected from the return.

Co-authored-by: Jubilee <[email protected]>
@workingjubilee
Copy link
Member

just a squash

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 8, 2025

📌 Commit 4457f44 has been approved by workingjubilee

It is now in the queue for this repository.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 8, 2025
…=workingjubilee

Document `Sum::sum` returns additive identities for `[]`

Because the neutral element of `<fNN as iter::Sum>` was changed to `neg_zero`, the documentation needed to be updated, as it was reporting inadequate information about what should be expected from the return.

Relevant Commit: rust-lang@4908188
Relevant Pull Request: rust-lang#129321

---

The referenced commit causes unintended side effects on presentation layer applications like using Tera templates, for example. I'm not sure what the motivation was behind the original change, but it seems like more discussion should be put into this issue and potentially have that change reverted.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2025
…kingjubilee

Rollup of 13 pull requests

Successful merges:

 - rust-lang#134999 (Add cygwin target.)
 - rust-lang#135439 (Make `-O` mean `OptLevel::Aggressive`)
 - rust-lang#136397 (Add a comment pointing to ICE-136223)
 - rust-lang#136681 (resolve `llvm-config` path properly on cross builds)
 - rust-lang#136686 (Clean up `HashMap` and `HashSet` docs.)
 - rust-lang#136694 (Update minifier version to `0.3.4`)
 - rust-lang#136706 (compiler: mostly-finish `rustc_abi` updates)
 - rust-lang#136710 (Document `Sum::sum` returns additive identities for `[]`)
 - rust-lang#136724 (Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]`)
 - rust-lang#136727 (Have a break from review rotation)
 - rust-lang#136730 (transmutability: fix ICE when passing wrong ADT to ASSUME)
 - rust-lang#136736 (Small resolve refactor)
 - rust-lang#136746 (Emit an error if `-Zdwarf-version=1` is requested)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 9, 2025
…=workingjubilee

Document `Sum::sum` returns additive identities for `[]`

Because the neutral element of `<fNN as iter::Sum>` was changed to `neg_zero`, the documentation needed to be updated, as it was reporting inadequate information about what should be expected from the return.

Relevant Commit: rust-lang@4908188
Relevant Pull Request: rust-lang#129321

---

The referenced commit causes unintended side effects on presentation layer applications like using Tera templates, for example. I'm not sure what the motivation was behind the original change, but it seems like more discussion should be put into this issue and potentially have that change reverted.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2025
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#136397 (Add a comment pointing to ICE-136223)
 - rust-lang#136681 (resolve `llvm-config` path properly on cross builds)
 - rust-lang#136686 (Clean up `HashMap` and `HashSet` docs.)
 - rust-lang#136694 (Update minifier version to `0.3.4`)
 - rust-lang#136706 (compiler: mostly-finish `rustc_abi` updates)
 - rust-lang#136710 (Document `Sum::sum` returns additive identities for `[]`)
 - rust-lang#136724 (Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]`)
 - rust-lang#136727 (Have a break from review rotation)
 - rust-lang#136730 (transmutability: fix ICE when passing wrong ADT to ASSUME)
 - rust-lang#136736 (Small resolve refactor)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2025
…kingjubilee

Rollup of 8 pull requests

Successful merges:

 - rust-lang#136397 (Add a comment pointing to ICE-136223)
 - rust-lang#136686 (Clean up `HashMap` and `HashSet` docs.)
 - rust-lang#136706 (compiler: mostly-finish `rustc_abi` updates)
 - rust-lang#136710 (Document `Sum::sum` returns additive identities for `[]`)
 - rust-lang#136724 (Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]`)
 - rust-lang#136727 (Have a break from review rotation)
 - rust-lang#136730 (transmutability: fix ICE when passing wrong ADT to ASSUME)
 - rust-lang#136736 (Small resolve refactor)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c309089 into rust-lang:master Feb 9, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 9, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 9, 2025
Rollup merge of rust-lang#136710 - JakenHerman:jaken/iterator-docs, r=workingjubilee

Document `Sum::sum` returns additive identities for `[]`

Because the neutral element of `<fNN as iter::Sum>` was changed to `neg_zero`, the documentation needed to be updated, as it was reporting inadequate information about what should be expected from the return.

Relevant Commit: rust-lang@4908188
Relevant Pull Request: rust-lang#129321

---

The referenced commit causes unintended side effects on presentation layer applications like using Tera templates, for example. I'm not sure what the motivation was behind the original change, but it seems like more discussion should be put into this issue and potentially have that change reverted.
@JakenHerman JakenHerman deleted the jaken/iterator-docs branch February 11, 2025 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants