[math] remove implicit conversions to/from KahanSum #11941
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
egpbos
requested review from
guitargeek,
hageboeck and
lmoneta
as code owners
|
Starting build on |
Contributor
|
Very nice, thanks a lot! This always annoyed me too |
Contributor
Author
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
|
Ah, I know what the problem is there (I commented that line out in my local build, that's why it didn't fail for me). I'll fix the commit up later. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
egpbos
force-pushed
the
KahanSum_no_implicit_conversions
branch
from
ad15050 to
441bee5
Compare
|
Starting build on |
Contributor
Author
|
Rebased on #11940 branch (it was more convenient to leave it based on that after all, I tried taking the commit out, but it needed too many changes) and fixed the build error. |
guitargeek
approved these changes
Dec 21, 2022
Contributor
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
Thanks again for doing this!
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Errors:
Warnings:
And 73 more |
egpbos
force-pushed
the
KahanSum_no_implicit_conversions
branch
from
441bee5 to
abf72d6
Compare
|
Starting build on |
Contributor
Author
|
Together with @guitargeek, we found and fixed (in commit abf72d6) a bug in RooNLLVarNew, accidentally proving the usefulness of this PR 😄 |
egpbos
force-pushed
the
KahanSum_no_implicit_conversions
branch
from
abf72d6 to
5111af4
Compare
|
Starting build on |
Contributor
Author
|
The last rebase doesn't change any contents, just made it consistent with the branch in #11940. Still need to wait for CI. |
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Errors:
Warnings:
And 281 more |
|
Build failed on ROOT-debian10-i386/soversion. Errors:
Warnings:
And 175 more |
Contributor
Author
|
Ok, roottest has some failures too. I'll see whether I can fix those too. |
|
Build failed on mac12/noimt. Errors:
Warnings:
And 210 more |
|
Build failed on mac11/cxx14. Errors:
Warnings:
And 125 more |
|
Build failed on ROOT-ubuntu2004/python3. Errors:
Warnings:
And 286 more |
egpbos
added a commit
to roofit-dev/roottest
that referenced
this pull request
Dec 21, 2022
KahanSum objects will no longer be implicitly converted to their underlying type after root-project/root#11941. This commit explicitly converts them.
Contributor
Author
|
I fixed roottest, see root-project/roottest#928. I think after that this can be merged. |
52 tasks
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
Warnings:
And 269 more |
The auto-conversion to type T and implicit type T constructor in KahanSum make it hard to debug KahanSum, because it is easy to overlook implicit conversions in code, especially in lines where the type of the return value is `auto`. This commit removes them and where they were necessary replaces them with an explicit construction or explicit conversion to double via Sum().
The boolean flag accidentally got replaced in a previous commit.
egpbos
force-pushed
the
KahanSum_no_implicit_conversions
branch
from
5111af4 to
e975104
Compare
|
Starting build on |
egpbos
added a commit
to root-project/roottest
that referenced
this pull request
Dec 21, 2022
KahanSum objects will no longer be implicitly converted to their underlying type after root-project/root#11941. This commit explicitly converts them.
egpbos
added a commit
to root-project/roottest
that referenced
this pull request
Dec 21, 2022
KahanSum objects will no longer be implicitly converted to their underlying type after root-project/root#11941. This commit explicitly converts them.
Contributor
Author
|
@phsft-bot build |
|
Starting build on |
Contributor
Author
|
The roottest fixes were merged, so now CI should pass here. Also rebased on master, so the previous KahanSum PR commits are now gone from this PR. After CI passes, this can be merged. |
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Warnings:
And 298 more Failing tests:
|
|
Build failed on mac12/noimt. Warnings:
And 53 more Failing tests:
|
|
Build failed on mac11/cxx14. Errors:
Warnings:
And 54 more Failing tests:
And 8 more |
|
Build failed on ROOT-ubuntu2004/python3. Warnings:
And 54 more Failing tests:
|
|
Build failed on windows10/cxx14. Failing tests: |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
Warnings:
And 130 more Failing tests:
And 10 more |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull request:
Follow-up on #11940;
please merge that one before this oneit has been merged, so this one can also be merged.Changes or fixes:
The auto-conversion to type
Tand implicit typeTconstructor inKahanSummake it hard to debugKahanSum, because it is easy to overlook implicit conversions in code, especially in lines where the type of the return value isauto. The second commit in this PR (the first commit is the same as that in #11940) removes them and where they were necessary replaces them with an explicit construction or explicit conversion to double viaSum()Checklist: