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

Add pow method #21

Merged
merged 1 commit into from Jun 18, 2018
Merged

Add pow method #21

merged 1 commit into from Jun 18, 2018

Conversation

jeking3
Copy link
Contributor

@jeking3 jeking3 commented Jun 14, 2018

The original author of #9 deleted their repository. I applied the change manually.

This closes #9

@jeking3 jeking3 self-assigned this Jun 14, 2018
@jeking3 jeking3 requested review from swatanabe and mclow June 14, 2018 12:47
@@ -146,6 +146,9 @@ class rational
struct helper { IntType parts[2]; };
typedef IntType (helper::* bool_type)[2];

class already_normalized {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a public constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can leave it this way for now (private).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out this is no longer necessary with the removal of reciprocal().

@mclow
Copy link
Contributor

mclow commented Jun 14, 2018

Other than my one comment, this looks fine. I'd be really tempted to remove the Metrowerks 8.3 workarounds, though. That compiler was released in 2002/2003.

@codecov
Copy link

codecov bot commented Jun 14, 2018

Codecov Report

Merging #21 into develop will decrease coverage by 0.26%.
The diff coverage is 58.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #21      +/-   ##
===========================================
- Coverage    67.52%   67.25%   -0.27%     
===========================================
  Files            1        1              
  Lines          271      281      +10     
  Branches        90       97       +7     
===========================================
+ Hits           183      189       +6     
  Misses           3        3              
- Partials        85       89       +4
Impacted Files Coverage Δ
include/boost/rational.hpp 67.25% <58.33%> (-0.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70fe050...20ea6ba. Read the comment docs.

@swatanabe
Copy link
Contributor

swatanabe commented Jun 14, 2018 via email

@jeking3
Copy link
Contributor Author

jeking3 commented Jun 14, 2018

@swatanabe I'm not sure I understand what you mean by the 1/x in the boost::rational context. Without a reciprocal() call, the only way I see to do it otherwise is to call 1.0/rational_cast<double>(r), and then you are stuck with a double and no good way to get back to a rational<>IntType> again.

@swatanabe
Copy link
Contributor

swatanabe commented Jun 16, 2018 via email

@jeking3 jeking3 force-pushed the powrecip branch 2 times, most recently from c097503 to c6d9a20 Compare June 16, 2018 17:30
@jeking3
Copy link
Contributor Author

jeking3 commented Jun 16, 2018

I removed reciprocal() and used your suggestion, so the addition is just pow(..) now.

@jeking3 jeking3 changed the title Add pow and reciprocal methods Add pow method Jun 17, 2018
@jeking3 jeking3 merged commit 25ad8e5 into boostorg:develop Jun 18, 2018
@jeking3 jeking3 deleted the powrecip branch June 18, 2018 01:18
@jeking3 jeking3 added this to the v1.68.0 milestone Jun 18, 2018
@jeking3
Copy link
Contributor Author

jeking3 commented Jul 11, 2018

It looks like this may have caused some issues in poorly scoped calls to pow() - see:
qtumproject/cpp-eth-qtum#31

Any suggestions? Should Boost.Spirit be more specific?
Does pow need to use SFINAE to disable itself like the operators?
I think pow is being defined in namespace boost which means all other boost libraries will pick it up if they include the header and then call an unqualified pow(...).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants