Drop scratchbuf#239
Conversation
|
@stoeckmann this is what I mentioned some time ago about doing with the scratchbuf impl. |
lucasdemarchi
force-pushed
the
drop-scratchbuf
branch
from
ca89bc7 to
aa19441
Compare
evelikov
left a comment
evelikov
left a comment
There was a problem hiding this comment.
Have a slight inclination towards not using alloca plus the strbuf_reserve() instance added at the end seems off.
As a whole, I like the idea - esp the robustness fixes for strbuf_steal()
| } | ||
|
|
||
| static bool buf_grow(struct strbuf *buf, size_t newsize) | ||
| bool strbuf_reserve(struct strbuf *buf, size_t n) |
There was a problem hiding this comment.
Would be great if we can have a way to instrument the compiler/analyzers that follow-up strbuf_str() and strbuf_push*() calls cannot fail, when reservation large enough. As humans we'll likely get it wrong at some point.
Any clever trick that comes to mind?
There was a problem hiding this comment.
My understanding of strbuf so far is that it abstracts all the memory allocation handling away from the caller who can fully concentrate on the actual business logic. The only condition is: Check return values of strbuf functions.
It's no surprise to see this function since scratchbuf functionality has been merged into strbuf, but ... do we really need it (non-statically) and is is a good addition to the API?
There was a problem hiding this comment.
I think it's worthy. In several places we have the strategy "1) calculate the size; 2) allocate; 3) use it". Allowing to reserve space is IMO a good thing to do
stoeckmann
left a comment
stoeckmann
left a comment
There was a problem hiding this comment.
I like the approach. Ideally we can keep the API in a way that all return values have to be checked for error to never accidentally fool ourselves through reserving "enough" memory and eventually figuring out that our implicit code contains an error.
The cleanup attribute is awesome and I never expected to see a good alloca usage. If we want to keep alloca though, I'm rather indifferent.
No good idea so far if we could somehow make the API more robust against accidentally calling strbuf_str on a strbuf with stack memory and returning it from a function or storing it somewhere in a heap struct. But perhaps we have to live with it and actually have to think what we are doing here. :)
| } | ||
|
|
||
| static bool buf_grow(struct strbuf *buf, size_t newsize) | ||
| bool strbuf_reserve(struct strbuf *buf, size_t n) |
There was a problem hiding this comment.
My understanding of strbuf so far is that it abstracts all the memory allocation handling away from the caller who can fully concentrate on the actual business logic. The only condition is: Check return values of strbuf functions.
It's no surprise to see this function since scratchbuf functionality has been merged into strbuf, but ... do we really need it (non-statically) and is is a good addition to the API?
lucasdemarchi
force-pushed
the
drop-scratchbuf
branch
from
aa19441 to
93cd70a
Compare
That's the same situation of declaring a buffer on stack and returning a pointer to it. I guess all the sanitizers we have can help us with that. If not, at least valgrind is here to the rescue :) |
|
Please take a look in the last commit. That would be a replacement for the alloca. If we feel it's a better approach I can polish it more. |
lucasdemarchi
force-pushed
the
drop-scratchbuf
branch
2 times, most recently
from
eecb1a8 to
cec07d1
Compare
Extract the realloc from the size calculation so it can be shared with other upcoming callers. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
Let the realloc happen in only one place, so we can change its behavior in future. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
Document the behavior for these functions and also clarify why the testsuite can check the buffer for equality after calling strbuf_str(). Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
The main for strbuf_steal() to free() on error was to allow the caller to pass the NULL up the stack with just a return call to strbuf_steal(). However this is error-prone and surprising that the buffer is still invalidated on error. Provide an strbuf cleanup attribute that can be used for the same purpose and make sure that when the string is stolen, it's set to NULL, so there's no dangling pointer around. Since we run the testsuite with AddressSanitizer, a simple test can be added to make sure the stolen string becomes valid when the attribute is used. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
lucasdemarchi
force-pushed
the
drop-scratchbuf
branch
from
cec07d1 to
affc52a
Compare
This is the main functionality of the scratchbuf implementation: it starts with a buffer on stack that covers most of the calls, but also has a fallback to allocate the buffer if it grows beyond the initial size. Port that functionality from scratchbuf to strbuf, so the former can be eventually removed. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
To accomplish the same task as scratchbuf_alloc() does: make sure the buffer has enough space for next operations. One difference is that ensures **extra** space, not the total space. If needed in future, a strbuf_reserve(), that resembles C++'s std::vector::reserve(), can be added on top. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
Unconditionally appending '\0' is not a big problem, but that does trigger the buffer to potentially be re-allocated. Avoid that by checking the last char is not already NUL. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
Wrapper for memcpy(). It's similar to strbuf_pushchars(), but push any char, including the NUL byte, relying on the size passed as argument. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
So users don't feel tempted to look at inside the strbuf. Just add a function for it and proper tests. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
Useful when working with paths on a loop and resetting to a base path component on every loop iteration. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
Replace the scratchbuf usage with corresponding API from strbuf. depmod_modules_search_path() itself may further be simplified in the future to share opening the dir with depmod_modules_search_dir(), but that is left for later. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
Remove last use of scratchbuf. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
All its unique features have been ported to strbuf and users converted. Nuke it. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com>
lucasdemarchi
force-pushed
the
drop-scratchbuf
branch
from
affc52a to
813bb1c
Compare
evelikov
left a comment
evelikov
left a comment
There was a problem hiding this comment.
One thing I forgot to mention, non-blocker:
When doing a release build the asserts() are omitted. Since we build libshared.a aka strbuf.o once, the xfail test likely will not trigger. One option is to move the function as inline to strbuf.h, then the explicit -UNDEBUG we add to the tests will keep the assert... Although we need to find a better solution for the long run.
Tl;Dr: xfail test will succeed on meson setup --buildtype release builds, move strbuf_shrink_to() to the header as WA.
Regardless of the above comment, lets land this PR - it's looking solid IMHO.
not rely on the assert from inside the lib... we should have an assert_return(), not affected by NDEBUG, to guarantee what we are checking. |
|
and it seems we need to review those asserts in strbuf.... so I'm leaving this as is for now. |
Extract the realloc from the size calculation so it can be shared with other upcoming callers. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
Let the realloc happen in only one place, so we can change its behavior in future. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
Document the behavior for these functions and also clarify why the testsuite can check the buffer for equality after calling strbuf_str(). Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
The main for strbuf_steal() to free() on error was to allow the caller to pass the NULL up the stack with just a return call to strbuf_steal(). However this is error-prone and surprising that the buffer is still invalidated on error. Provide an strbuf cleanup attribute that can be used for the same purpose and make sure that when the string is stolen, it's set to NULL, so there's no dangling pointer around. Since we run the testsuite with AddressSanitizer, a simple test can be added to make sure the stolen string becomes valid when the attribute is used. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
This is the main functionality of the scratchbuf implementation: it starts with a buffer on stack that covers most of the calls, but also has a fallback to allocate the buffer if it grows beyond the initial size. Port that functionality from scratchbuf to strbuf, so the former can be eventually removed. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
To accomplish the same task as scratchbuf_alloc() does: make sure the buffer has enough space for next operations. One difference is that ensures **extra** space, not the total space. If needed in future, a strbuf_reserve(), that resembles C++'s std::vector::reserve(), can be added on top. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
Unconditionally appending '\0' is not a big problem, but that does trigger the buffer to potentially be re-allocated. Avoid that by checking the last char is not already NUL. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
Wrapper for memcpy(). It's similar to strbuf_pushchars(), but push any char, including the NUL byte, relying on the size passed as argument. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
So users don't feel tempted to look at inside the strbuf. Just add a function for it and proper tests. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
Useful when working with paths on a loop and resetting to a base path component on every loop iteration. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
Replace the scratchbuf usage with corresponding API from strbuf. depmod_modules_search_path() itself may further be simplified in the future to share opening the dir with depmod_modules_search_dir(), but that is left for later. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
Remove last use of scratchbuf. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
All its unique features have been ported to strbuf and users converted. Nuke it. Signed-off-by: Lucas De Marchi <lucas.de.marchi@gmail.com> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com> Link: #239
|
Applied, thanks for the reviews |
Counter intuitive of the name, the macro never |
3 participants
Slightly tested - probably need more CI and eyeballs.