m3m0ry / fixedpoint

Decimal fixed point type
Boost Software License 1.0
6 stars 3 forks source link

constructor should throw or clamp on overflow #1

Closed schveiguy closed 4 years ago

schveiguy commented 4 years ago
auto fd = FixedDecimal!10("12345678901234567");
writeln(fd); // 2785546338.0256111616

I would expect an error if the string can't fit in the type (like std.conv.to does in this case).

schveiguy commented 4 years ago

I was going to add some more issues to the string constructor, but I think instead I'll just make a PR ;)

Note that your master branch has changed the module name from the latest release, making it very confusing to use (no docs are available, so I didn't know, had to look at the code in my dub cache to notice).

m3m0ry commented 4 years ago

Hi and thanks for the suggestions. I didn't see that you have created an issue and now there are two. I will tinker with it for 1-2 weeks. I think PRs are too early at this stage, since i am still pushing code around, and renaming modules/packages.

Right now i plan to have more then one class. (maybe some functions). I still want to keep it dead simple (so no IEEE-Decimal) but with more functionality.

m3m0ry commented 4 years ago

I have fixed this bug, but i am not satisfied with my solution. I would like the class to be usable without gc, for compile time execution.

schveiguy commented 4 years ago

GC is ok in compile-time execution.

schveiguy commented 4 years ago

I like the idea in your solution, to reuse to's overflow mechanisms, clever! But you can do it without allocation:

value = spl[0].chain('0'.repeat(scaling)).to!long;

And actually, it would be good to templatize the string for Char type on the constructor. And actually, if you are going to do that, just templatize on a range of any character.

schveiguy commented 4 years ago

Hm... split is allocating as well. But we can do it with splitter instead! I'll try and make a @nogc version (save for the exception throwing possibility).

m3m0ry commented 4 years ago

Alright. I think i have implemented all suggestions. I am not sure if it's nogc. What is the fastest way to "prove" that? The only thing: Fixed!3("123.1234") won't produce an error, rather a number "123.123". My reasoning was, the user wanted to truncate some numbers.

And regarding using to's overflow mechanisms, i am not sure if it is that clever. At least i would be confused with the exception, if i didn't know about it.

Also i would like to emphasize that the interaction with you is really great. I am grateful of your improving suggestions. It is surprising to myself, how much it motivates, when somebody else shows interest in my code, especially if it's public & open source.

m3m0ry commented 4 years ago

Any more wishes?

schveiguy commented 4 years ago

I've been a bit busy lately. I'll check back on this in a couple days. Can't right now.

schveiguy commented 4 years ago

Check out #3. I think that's about it for me!