Added sqrt functions #11
Conversation
|
Can you tell me which OS and compiler you use? |
|
I use Windows OS with QT Creator compiler |
include/functions/math.hpp
Outdated
| } | ||
| } | ||
| } else { | ||
| throw std::invalid_argument("Expected a non-negative BigInt"); |
There was a problem hiding this comment.
This should be at the beginning of the function:
if (num < 0)
throw std::invalid_argument("Cannot compute square root of a negative integer");Then you wouldn't need to enclose the rest of the body inside if (num >= 0) { ... }.
test/functions/math.test.cpp
Outdated
| BigInt big_num1 = "3479680743635798438602889954485038919559460388450"; | ||
| REQUIRE(sqrt(big_num1) == "1865390238967653234193278"); | ||
|
|
||
| BigInt big_num2 = "471336517784520212796937741396686403167942977773121645803479339647737161"; |
There was a problem hiding this comment.
Create a single BigInt object and reuse it:
BigInt num = 12345;
...
num = 78910;
test/functions/math.test.cpp
Outdated
| REQUIRE(sqrt(big_num2) == "686539523832765372419381874316283619"); | ||
|
|
||
| BigInt big_num3 = "304953007708768585645827645127232631942308013843965557450677671087391006548030000"; | ||
| REQUIRE(sqrt(big_num3) == "17462903759362833573945713140457169848646"); |
There was a problem hiding this comment.
- Please add at least 5 more cases with numbers of different magnitudes - such as 5 digits, 20 digits, 100 digits, 500 digits etc.
- Verify base cases in a separate test case (as in case of
pow()). The base cases will check the square root of 0, 1 and -1.
test/functions/math.test.cpp
Outdated
| @@ -173,3 +173,15 @@ TEST_CASE("pow with string base", "[functions][math][pow][string]") { | |||
| "08862990907940498760592041482791966715057053380053636634477979263" | |||
| "4601932346249911644260403539535769743430516736"); | |||
| } | |||
|
|
|||
| TEST_CASE("Sqrt of big integers", "[functions][math][sqrt][big]") { | |||
| BigInt big_num1 = "3479680743635798438602889954485038919559460388450"; | |||
There was a problem hiding this comment.
I think this is where you're getting compilation errors. Please see #3 for workarounds.
Since I don't frequently use Windows (except for gaming 👾), I haven't yet tried compiling the library on it. But I'm logged on to Windows at the moment and will figure out a simple way to compile and will update you regarding it. Until then please make the changes requested in the review. |
|
Thanks Faheel, I've made the changes you requested. |
|
I tried using CMake for cross-platform building, and though it compiles with CLion on Windows, it may not work with other IDEs/compilers on Windows. If you have an IDE that supports CMake, then you can add CMakeLists.txt to the project's root folder and can compile on Windows from that IDE. If not, then what you can do for now is that when you feel that your changes look good, push them to GitHub and check out the Travis build log for any compile errors or test failures. Also, there was a problem with the test script that made Travis think that the tests had passed even if some test had failed. So the build was passing even though all the test cases for |
|
The test script is fixed now, so please update your fork (by pulling from upstream). |
Codecov Report
@@ Coverage Diff @@
## master #11 +/- ##
==========================================
- Coverage 96.5% 96.47% -0.04%
==========================================
Files 12 12
Lines 458 482 +24
==========================================
+ Hits 442 465 +23
- Misses 16 17 +1
Continue to review full report at Codecov.
|
|
@arvindvs Thanks a lot for contributing to the project! And sorry for the inconvenience caused by Windows builds not being supported. It will be supported soon (hopefully!). |
I have made updates to the math.hpp and math.test.cpp files, including 3 new test cases for the sqrt function.
Relates to #5.