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

Simplify conversions around arithmetic operators #270

Merged
merged 4 commits into from
May 24, 2022
Merged

Conversation

chfast
Copy link
Owner

@chfast chfast commented May 5, 2022

No description provided.

@chfast chfast changed the title Simplify conversions for operator+ Simplify conversions around binary arithmetic operators May 16, 2022
@chfast chfast changed the title Simplify conversions around binary arithmetic operators Simplify conversions around arithmetic operators May 16, 2022
@chfast chfast force-pushed the binop_simplify_poc branch 2 times, most recently from 6d79688 to eb0eae1 Compare May 16, 2022 20:35
@chfast chfast marked this pull request as ready for review May 16, 2022 20:55
@chfast chfast requested review from axic, gumb0 and yperbasis May 16, 2022 20:55
@codecov-commenter
Copy link

codecov-commenter commented May 16, 2022

Codecov Report

Merging #270 (1c3c141) into master (de3388d) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #270   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         1983      1956   -27     
=========================================
- Hits          1983      1956   -27     
Flag Coverage Δ
32bit 100.00% <100.00%> (ø)
gcc 99.48% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/intx/intx.hpp 100.00% <100.00%> (ø)

Copy link
Collaborator

@axic axic left a comment

Choose a reason for hiding this comment

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

Looks good, at least less operators to implement. Does this have an effect on speed?

@@ -1785,7 +1775,7 @@ inline void udivrem_knuth(
} // namespace internal

template <unsigned M, unsigned N>
div_result<uint<M>, uint<N>> udivrem(const uint<M>& u, const uint<N>& v) noexcept
constexpr div_result<uint<M>, uint<N>> udivrem(const uint<M>& u, const uint<N>& v) noexcept
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this constexpr just missed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes. Maybe we never used it in constexpr context but now compiler is not happy as we removed one level of templates and maybe it is obviously wrong now.

@chfast
Copy link
Owner Author

chfast commented May 24, 2022

No performance changes. In the end we define the same functions just is some other location.

@chfast chfast merged commit ea6a54f into master May 24, 2022
@chfast chfast deleted the binop_simplify_poc branch May 24, 2022 18:07
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.

3 participants