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

Add dectest suite and regex parser #18

Merged
merged 63 commits into from
Feb 11, 2019
Merged

Add dectest suite and regex parser #18

merged 63 commits into from
Feb 11, 2019

Conversation

joshcarp
Copy link
Contributor

dectest suite from http://speleotrove.com/decimal/dectest.html

Currently testing only tests ddAdd.dectest (double decimal, 64 bits)
Testcases are for following functions:
http://speleotrove.com/decimal/dectest.html

in response to issues:
#17 and #1

joshcarp added 17 commits January 5, 2019 11:30
@joshcarp
added some tests
46e10fa
@joshcarp
Merge remote-tracking branch 'upstream/master'
ef4cfde
@joshcarp
Overflow protection, rounding to inf in Add function
30652eb
@joshcarp
Overflow protection, rounding to inf in Add function
69f54ee
@joshcarp
fixed problems with last pull request
ed16296
@joshcarp
cleaned up tests
a4ba143
@joshcarp
fix Quo function div by zero
9e8c835
@joshcarp
fix Mul function infinities
bdafc16
@joshcarp
edit newFromParts to return infinity
b4d7d8e
@joshcarp
Edit style and test for infinity
7a9ecee
@joshcarp
fix newfromparts sum of zero and small numbers into big
fdc99ad
@joshcarp
Fix pull request based off feedback in anz-bank#14
73aeca3
@joshcarp
Fix global variable comment
69a33ba
@joshcarp
Move overflow protection to NewfromParts from arithmetic functions
fab891a
@joshcarp
Simplify newFromParts, inf protection moved to arithmetic functions
4afa09c
@joshcarp
Fix blank line
ddcd461
@joshcarp
Add dectest suite and regex parser
c6bf77c
@juliaogris juliaogris requested a review from camscale January 31, 2019 04:19
juliaogris
juliaogris suggested changes Jan 31, 2019
View reviewed changes
Copy link
Contributor

@juliaogris juliaogris left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Pretty cool that you managed to string this together!

Don't be disheartened by my many comments and feel free to give me pushback. Shouldn't take that long to adjust stuff anyway and rest assured @camh-anz or @anzdaddy will weigh in if they think I got it wrong ;)

One big concerned though: we don't have permissions to use these test cases quite yet I believe so we cannot merge this PR (please correct me if I'm wrong). You should probably add a big DISCLAIMER to the top of the PR description that you are trying to get permission for usage in this project and until then it can't be merged.

@anzdaddy might know better, but I believe:

-- Copyright (c) Mike Cowlishaw,  1981, 2010.  All rights reserved.   --
-- Parts copyright (c) IBM Corporation, 1981, 2008.                   --

without any additional clauses on usage means we are not allowed to use it in this repo.

decimal64.go Outdated Show resolved Hide resolved
decimal64_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
anzdaddy
anzdaddy requested changes Feb 2, 2019
View reviewed changes
Copy link
Member

@anzdaddy anzdaddy left a comment

Choose a reason for hiding this comment

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

Good job. A lot of nit-picky stuff, so don't worry too much about the number of review comments.

decimal64const.go Outdated Show resolved Hide resolved
decimal64math_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
juliaogris
juliaogris reviewed Feb 5, 2019
View reviewed changes
Copy link
Contributor

@juliaogris juliaogris left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me now, just waiting for a resolution with the test files.

marcelocantos
marcelocantos suggested changes Feb 6, 2019
View reviewed changes
decimalSuite_test.go Outdated Show resolved Hide resolved
.gitattributes Outdated Show resolved Hide resolved
decimal64const.go Show resolved Hide resolved
decimal64const.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Show resolved Hide resolved
@marcelocantos
Copy link

To keep this PR bounded, let's exclude tests that don't pass or are not executing, then bring them into subsequent PRs once they do.

@joshcarp
Copy link
Contributor Author

joshcarp commented Feb 7, 2019

To keep this PR bounded, let's exclude tests that don't pass or are not executing, then bring them into subsequent PRs once they do.

Going to coment out "TestFromSuite" function as a placeholder, already have a branch that passes all ddAdd operations, will bring in next PR

anzdaddy
anzdaddy requested changes Feb 7, 2019
View reviewed changes
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimal64const.go Outdated Show resolved Hide resolved
decimal64const.go Outdated Show resolved Hide resolved
marcelocantos
marcelocantos reviewed Feb 7, 2019
View reviewed changes
.gitattributes Outdated Show resolved Hide resolved
juliaogris
juliaogris approved these changes Feb 11, 2019
View reviewed changes
Copy link
Contributor

@juliaogris juliaogris left a comment

Choose a reason for hiding this comment

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

LGTM

@joshcarp joshcarp merged commit 2995332 into anz-bank:master Feb 11, 2019
joshcarp added a commit to joshcarp/decimal that referenced this pull request Sep 1, 2019
@joshcarp
Add dectest suite and regex parser (anz-bank#18)
63736ab 
* Add dectest suite
* Add regexparser for dectest suite
* fix error printing
* Add .gitattributes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelocantos marcelocantos marcelocantos requested changes

@juliaogris juliaogris juliaogris approved these changes

@anzdaddy anzdaddy anzdaddy approved these changes

@camscale camscale Awaiting requested review from camscale

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

Successfully merging this pull request may close these issues.
4 participants
@joshcarp @anzdaddy @marcelocantos @juliaogris