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 simplified implementation in subproject/simple #67

Merged
merged 17 commits into from
Aug 28, 2024

Conversation

tobybell
Copy link
Contributor

@tobybell tobybell commented Aug 24, 2024

Regarding #64; here's my first stab at simplifying your reference implementation. It's still not exactly short and sweet, but it's at least a bit less noisy with macros and templates.

This implementation supports both to_decimal and to_chars, and the binary_to_decimal_rounding, decimal_to_binary_rounding, and cache policies. It's all in one file, simple_dragonbox.h.

It also has its own test executable, simple_dragonbox_test.cpp, which implements the same tests as uniform_random_test and test_all_shorter_interval_cases from the primary implementation.

I arrived at this implementation mostly by refactoring your existing code, rather than writing a new implementation from scratch, so there may still be simplifications I didn't spot. Especially your main algorithm, which I essentially didn't touch at all.

I'm submitting this as what I've got so far, since I need to pause working on it for now. From your comment on issue #64, I think there are a few further changes you might prefer to make:

  • Remove support for policies altogether, and just use the default ones
  • Reuse the same unit test code between the primary implementation and the simple implementation

I might return to work on those in the future. Let me know if there's anything else you see here that I missed as a simplification you had in mind. I wasn't sure how much you wanted to remove special cases for e.g. division by powers of 10, etc.

Copy link
Owner

@jk-jeon jk-jeon left a comment

Choose a reason for hiding this comment

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

Overall, looks pretty good. Thanks a lot!
There are just a few things that does not align with my preferences.

subproject/simple/README.md Outdated Show resolved Hide resolved
subproject/simple/README.md Outdated Show resolved Hide resolved
subproject/simple/README.md Outdated Show resolved Hide resolved
subproject/simple/README.md Outdated Show resolved Hide resolved
subproject/simple/simple_dragonbox.h Outdated Show resolved Hide resolved
subproject/simple/simple_dragonbox.h Outdated Show resolved Hide resolved
subproject/simple/simple_dragonbox.h Show resolved Hide resolved
subproject/simple/simple_dragonbox.h Outdated Show resolved Hide resolved
subproject/simple/simple_dragonbox.h Outdated Show resolved Hide resolved
subproject/simple/simple_dragonbox.h Outdated Show resolved Hide resolved
@tobybell
Copy link
Contributor Author

tobybell commented Aug 26, 2024

Thanks for the prompt and detailed review. All your requested changes sound fine to me. I will implement them tomorrow (Monday) night.

@tobybell
Copy link
Contributor Author

I think I addressed each of your comments with one commit, in order.

@jk-jeon jk-jeon merged commit 90f96f3 into jk-jeon:master Aug 28, 2024
11 checks passed
@jk-jeon
Copy link
Owner

jk-jeon commented Aug 28, 2024

Merged, thanks!

@jk-jeon
Copy link
Owner

jk-jeon commented Aug 29, 2024

Just a few notes about commits I made after merging the PR (not the ones listed above):

  • I decided to wrap everything inside jkj:: namespace.
  • I added license boilerplates into the source files.
  • I removed your test file and rather merged it into the existing tests.
  • Explicit template specialization is forbidden in class. You can only do partial specializations. A typical workaround is to add a dummy template parameter, so I did that for cache_holder.
  • Added a mention of this in the main README.

Thanks again!

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.

2 participants