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

Implement the class function #47

Merged
merged 20 commits into from
Jun 19, 2019
Merged

Implement the class function #47

merged 20 commits into from
Jun 19, 2019

Conversation

MichaelDao
Copy link
Contributor

@MichaelDao MichaelDao commented May 21, 2019

As this is my first pull request to this project, I thought it would be good to improve the documentation so that when someone new like me starts, they can read the same documentation everyone else has read and learn more about the IEEE 754 revised floating point.

I implemented the class function (more info: http://speleotrove.com/decimal/damisc.html#refclass).
A miscellaneous function that takes in a decimal and returns a string of 10 possibilities:

  • "sNaN" (signaling NaN)
  • "NaN" (quiet NaN)
  • "-Infinity" (negative infinity)
  • "-Normal" (negative normal finite number)
  • "-Subnormal" (negative subnormal finite number)
  • "-Zero" (negative zero)
  • "+Zero" (non-negative zero)
  • "+Subnormal" (positive subnormal finite number)
  • "+Normal" (positive normal finite number)
  • "+Infinity" (positive infinity).

Also made a minor change to printout from the test suite to space out the logging.

@MichaelDao
Copy link
Contributor Author

MichaelDao commented May 21, 2019

Had code reviewed by @joshcarp in person and have applied the suggestions:

  • Fixed the issues picked up by gometalinter.
  • Refactored the class function to concatenate "+" or "-" onto the returning string.
  • Unpacked the decimal into decparts and utilised private functions to avoid having to unpack it each time with public functions.
  • Created more private functions e.g. isSubnormal() for use with decparts, reorganized them so that they are next to other relevant private functions

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
@MichaelDao MichaelDao force-pushed the ddclass branch 2 times, most recently from 30a575e to f519c09 Compare May 22, 2019 03:33
@MichaelDao
Copy link
Contributor Author

I have made all the recommended changes suggested by @anzdaddy

  • The code in the documentation has been given syntax highlighting as well as minor fixups.
  • The comment for the class function is now complete, along with a link reference to more details.
  • Quickly fixed the way we pass parameters to the Class() function, is now matching the function in the documented standard

decimal64.go Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64const.go Outdated Show resolved Hide resolved
decimalSuite_test.go Outdated Show resolved Hide resolved
@anzdaddy
Copy link
Member

anzdaddy commented Jun 1, 2019

(Damn, I've got to stop doing reviews as @marcelocantos accidentally!)

decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
anzdaddy
anzdaddy previously approved these changes Jun 10, 2019
@MichaelDao MichaelDao changed the title Implement the class function and expand the readme documentation Implement the class function Jun 11, 2019
Copy link
Contributor

@joshcarp joshcarp left a comment

Choose a reason for hiding this comment

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

Some changes still needed, Looks alright though

decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
@joshcarp
Copy link
Contributor

joshcarp commented Jun 12, 2019

Hey missed the pointer receiver thing. Change all the decParts methods from
func (dec decParts) name()
to
func (dec *decParts) name()
This means that the decParts instance won't be copied into the local namespace, but will use the existing memory allocation of dec
This means that its faster, but also means that the dec is mutable

decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
decimal64.go Outdated Show resolved Hide resolved
joshcarp
joshcarp previously approved these changes Jun 19, 2019
decimal64.go Outdated
@@ -474,3 +498,30 @@ func propagateNan(d ...*decParts) *Decimal64 {
}
return nil
}

// Class is a miscellaneous operation that takes one operand and provides the class the decimal is in.
// This function is formally documented here http://speleotrove.com/decimal/damisc.html#refclass.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove link

Copy link
Contributor

@joshcarp joshcarp left a comment

Choose a reason for hiding this comment

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

Slight pr creep but all good,
There seems to be some more problems with using outdated methods, but that'll be fixed in the next PR

@joshcarp joshcarp merged commit 96cf674 into anz-bank:master Jun 19, 2019
@joshcarp joshcarp mentioned this pull request Jun 21, 2019
joshcarp pushed a commit to joshcarp/decimal that referenced this pull request Sep 1, 2019
* Add class function, detect NaN and sNaN

* Defined class for negatives, Normals and Infinities

* Add subnormal function, complete class function

* Add expmin const, refactor class function & add original unit tests back in

* Add missing private methods for flavours

* Move decparts methods to decimal64decParts.go

* Move decParts tests to own file
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.

4 participants