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

Public interface should provide safe buffer lengths #149

Open
ecorm opened this issue Jan 14, 2020 · 24 comments
Open

Public interface should provide safe buffer lengths #149

ecorm opened this issue Jan 14, 2020 · 24 comments

Comments

@ecorm
Copy link

ecorm commented Jan 14, 2020

There should be #define macros in rhu.h which specifies safe buffer lengths for the buffered functions.

@ecorm
Copy link
Author

ecorm commented Jan 15, 2020

@expnkx Why not? Users currently have to copy and paste a magic number buried inside an internal source file. If the buffer size needs to change for some reason, those using the macro will automatically use the correct length upon recompilation.

Unless something was added to the latest C standards, C does not provided a zero-cost numeric constant declaration other than #define.

@ecorm
Copy link
Author

ecorm commented Jan 15, 2020

Macro itself is a problem. It always pollutes every code it touches.

I don't deny that, but there's no other zero-cost alternative in C (this library is written in C, and I want to use this library). With a reasonable prefix, the risk of pollution can be mitigated. "Don't hard-code arbitrary magic numbers" is a common coding standard.

At the very least, the safe buffer size constant could be put in a separate header file that the user can optionally #include.

bufferred_size will seriously harm the performance of this library.

I'm not proposing to change the internal workings of this library. I'm just proposing that the magic number 25 be #defined somewhere instead of having to manually inspect d2s.c line 506 on every release and pray that it hasn't changed.

numeric algorithms like ryu often can only figure out how much buffer lengths after it finishes calculations.

Isn't dynamic allocation magnitudes slower than using a fixed-size buffer known at compile-time? It can also lead to heap fragmentation in small embedded environments.

If I want to stringify thousands of floats in a loop using this library, reusing the same 25-byte buffer makes more sense than having this library dynamically allocate that buffer thousands of times.

Of course I don't know the length of each stringified float, but there is a known maximum length and I want this library to provide me a compile-time symbol with the maximum possible length.

Just so we're clear, my proposal (as well as my other recent ones), pertains to this library only. Not your implementation of Ryu of anybody else's. You can of course do whatever you want with your library.

@ecorm
Copy link
Author

ecorm commented Jan 15, 2020

You are free to disagree on technical matters, but I will no longer tolerate any personal attacks from you.

@ecorm
Copy link
Author

ecorm commented Jan 15, 2020

@ulfjack I have no way of contacting you privately, so I'm posting this here. You have a problem here with a user who keeps harassing the participants of your issue tracker with personal insults (which I have archived offline). That user is also actively discouraging the use of your library. I recommend that you take the appropriate measures, or your user base will end up turning away from this project: https://help.github.com/en/github/building-a-strong-community/blocking-a-user-from-your-personal-account

@ecorm
Copy link
Author

ecorm commented Jan 15, 2020

@ulfjack I can provide you with screenshots of this issue thread before the user in question deleted the worst of the personal insults. If you have email notifications enabled, please review the original transcript of this thread.

@ulfjack
Copy link
Owner

ulfjack commented Jan 15, 2020

Sorry about that, @ecorm. I've blocked @expnkx now. @expnkx, this isn't acceptable behavior in my repositories. I think you have some great technical insights, but you can't behave like this.

@jeaiii
Copy link

jeaiii commented Oct 2, 2020

for c the choices are limited, maybe something like this:

struct ryu_float_buffer { char buffer[sizeof("-1.23456789e-12")]; };
struct ryu_double_buffer { char buffer[sizeof("-1.2345678901234567e-123")]; };

then use the buffer directly or sizeof(struct ryu_float_buffer)

@ecorm
Copy link
Author

ecorm commented Oct 2, 2020

for c the choices are limited, maybe something like this:

struct ryu_float_buffer { char buffer[sizeof("-1.23456789e-12")]; };
struct ryu_double_buffer { char buffer[sizeof("-1.2345678901234567e-123")]; };

then use the buffer directly or sizeof(struct ryu_float_buffer)

@jeaiii That's a clever way of avoiding macros while being clear about the intent. The only problem is that the string literal could possibly allocate a small amount of static memory. This is probably not a problem for most, but could be for some.

How about this instead?

struct ryu_float_buffer { char buffer[16]; }; // -1.23456789e-12\0
struct ryu_double_buffer { char buffer[25]; }; // -1.2345678901234567e-123\0

@ecorm
Copy link
Author

ecorm commented Oct 2, 2020

then use the buffer directly or sizeof(struct ryu_float_buffer)

That would include padding bytes, but would be guaranteed to be sufficiently large. Obtaining the size of a struct member in C is a bit convoluted: https://stackoverflow.com/questions/3553296/sizeof-single-struct-member-in-c

@jeaiii
Copy link

jeaiii commented Oct 2, 2020

Yes, had to unlearn a lot to see what works in plain c! might go with this then, if concerned with strange padding scenarios,

typedef char ryu_float_buffer[sizeof("-1.23456789e-12")];
typedef char ryu_double_buffer[sizeof("-1.2345678901234567e-123")];

@ecorm
Copy link
Author

ecorm commented Oct 2, 2020

I was just about to propose C array typedefs myself.

To be clear, these type definitions would not be part of the ryu function signatures, but merely provided to allow the client to know (programmaticly) how large the buffers should be.

@paulharris
Copy link

paulharris commented Apr 1, 2021

then use the buffer directly or sizeof(struct ryu_float_buffer)

That would include padding bytes, but would be guaranteed to be sufficiently large. Obtaining the size of a struct member in C is a bit convoluted: https://stackoverflow.com/questions/3553296/sizeof-single-struct-member-in-c

I think you can specify padding of structs? I haven't done it in years.

I personally would be fine with a macro, or even a comment in ryu.h !
It is also not clear how big the buffer needs to be for d2fixed() and friends...
It seems to expect a 2000 byte buffer?

Would love to see some documentation in the header file at the very least, so I know just from looking at the header file if my assumptions are correct. ie I had to read the code to check if d2exp() used malloc or an internal static buffer, etc.
eg some libraries use a thread-specific static buffer, and the caller would copy the results out.

Would also be nice for the documentation to mention why d2s() has no precision option.

Would be nice if there was a d2s(precision) option, the naive implementation would be to generate a fixed and an exp result at that precision and then return the shortest ... that would work, right?

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

No branches or pull requests

5 participants
@paulharris @ulfjack @ecorm @jeaiii and others