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.

Sign up for GitHub

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

Jump to bottom

Math util refactor #4856

Merged
merged 5 commits into from
Jul 23, 2022
Merged

Math util refactor #4856

merged 5 commits into from
Jul 23, 2022

Conversation

Swiftb0y
Copy link
Member

Various fixes. I spend way too much time on this tbh.

@Swiftb0y
fix(math): roundUpToPowerOf2 was depending on UB:
f03b4b8 
It assumed that a signed integer could overflow which is UB
and thus the compiler optimizes anything that depended on that
out. The result was that if the power of two is actually greater
than representable, the function would go into an infinite loop
@Swiftb0y Swiftb0y marked this pull request as ready for review July 14, 2022 23:45
@Swiftb0y Swiftb0y force-pushed the math-util-refactor branch 2 times, most recently from 8cdded8 to 662c8dd Compare July 15, 2022 00:15
@Swiftb0y
Copy link
Member Author

I'm puzzled why the build is failing. The feature test macros don't seem to work. Even though they are C++20, on C++17 they should just be undefined and thus the fallback code should be generated...

@Swiftb0y Swiftb0y force-pushed the math-util-refactor branch from 662c8dd to 40ba1a6 Compare July 15, 2022 01:17
@Swiftb0y Swiftb0y force-pushed the math-util-refactor branch from 40ba1a6 to 6d7c776 Compare July 15, 2022 12:25
daschuer
daschuer reviewed Jul 15, 2022
View reviewed changes
src/util/math.h Outdated Show resolved Hide resolved
daschuer
daschuer reviewed Jul 15, 2022
View reviewed changes
src/util/math.h Outdated Show resolved Hide resolved
daschuer
daschuer requested changes Jul 15, 2022
View reviewed changes
src/util/math.h Show resolved Hide resolved
@Swiftb0y
Copy link
Member Author

I really don't want to spend more time on a change that will be obsolete in three weeks!
Merge this PR or leave it. Implement your suggested changes and I will review them, but this PR won't see another commit from me.

@daschuer
Copy link
Member

Here are my changes: Swiftb0y#7
Does this approach work for you?

daschuer
daschuer reviewed Jul 21, 2022
View reviewed changes
src/util/math.h Outdated Show resolved Hide resolved
daschuer
daschuer reviewed Jul 21, 2022
View reviewed changes
src/util/math.h Outdated Show resolved Hide resolved
@Swiftb0y
Copy link
Member Author

Code builds and ifdefs work now. wdyt @daschuer? Should we merge #4857 first so CI verifies both code-paths?

@daschuer
Copy link
Member

The Arch build is IMHO sufficient to test the second code path. No need to put more work into this. We may verify this in the main after merge. Should I press merge?

@Swiftb0y
Copy link
Member Author

right, forgot we had that. Yes, please press merge. Thank you for taking the time to find a compromise with me.

@daschuer daschuer merged commit 04b2198 into mixxxdj:main Jul 23, 2022
@Swiftb0y Swiftb0y deleted the math-util-refactor branch July 23, 2022 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daschuer daschuer daschuer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Swiftb0y @daschuer