removed usage of static buffers#7002
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7002 +/- ##
==========================================
- Coverage 98.69% 98.69% -0.01%
==========================================
Files 79 79
Lines 14677 14675 -2
==========================================
- Hits 14486 14484 -2
Misses 191 191 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
badasahog
requested review from
MichaelChirico and
ben-schwen
and removed request for
MichaelChirico and
aitap
| if (ndigits == 0 || (fsize == (fsize >> shift << shift))) { | ||
| if (i < sizeof(suffixes)) { | ||
| snprintf(output, sizeof(output), "%"PRIu64"%cB (%"PRIu64" bytes)", // # notranslate | ||
| snprintf(output, 100, "%"PRIu64"%cB (%"PRIu64" bytes)", // # notranslate |
There was a problem hiding this comment.
That's a lot of magic numbers to introduce into the code. Isn't there a better solution?
There was a problem hiding this comment.
sizeof(output) is just sizeof(char) in this context
There was a problem hiding this comment.
Well, sizeof(char*) to be precise, but I see what you mean. And yet now the code, all of the callers and multiple places in the callee, has to remember the size of the buffer. Isn't the cure worse than the disease?
| char *ch2 = ptr; | ||
| limit = imin(limit, 500); | ||
| static const char* strlim(const char *ch, char buf[static 500], size_t limit) { | ||
| char *ch2 = buf; |
There was a problem hiding this comment.
Instead of making the caller always promise a 500-byte buffer, sometimes to fit only 30 characters, why not make strlim accept size_t limit, char buf[limit]? Just make sure there's no off-by-one error when limit is equal to the size of the buffer.
There was a problem hiding this comment.
that would involve using a VLA, which I believe is against the rules of this project?
There was a problem hiding this comment.
we can change buf to be a standard pointer, which would allow passing in a smaller buffer, but I'm not sure that would save memory because it's a temporary stack allocation.
Plus, we would lose compile time checks.
There was a problem hiding this comment.
It wouldn't create a VLA if such a declaration shows up in the function arguments. The point about the compile time checks is taken, thanks.
| if (ndigits == 0 || (fsize == (fsize >> shift << shift))) { | ||
| if (i < sizeof(suffixes)) { | ||
| snprintf(output, sizeof(output), "%"PRIu64"%cB (%"PRIu64" bytes)", // # notranslate | ||
| snprintf(output, 100, "%"PRIu64"%cB (%"PRIu64" bytes)", // # notranslate |
There was a problem hiding this comment.
Well, sizeof(char*) to be precise, but I see what you mean. And yet now the code, all of the callers and multiple places in the callee, has to remember the size of the buffer. Isn't the cure worse than the disease?
|
@aitap the callers already had to give a size limit, that hasn't changed. Only the callee has changed. |
|
Please reopen this PR on a branch on this repo. It is required for any PR purporting performance effects to go through the atime suite. |
|
@MichaelChirico I don't believe I have permission to create branches? |
You should -- all Members should have branch write permission: https://github.com/orgs/Rdatatable/teams/project-members/members
|
|
@MichaelChirico done #7005 |
3 participants
Using a statically allocated buffer to avoid breaking lifetime limitations is unsafe. It's always better to pass the buffer in as a parameter.
It's also faster to do it this way, static storage duration is slow and can't be aliased with other memory nearly as easily.