Buffer growing cleanups#4255
Conversation
5b8227a to
db42831
Compare
|
Seems reasonable to me. One thing I noticed here is that the Notice the |
|
I'm going to merge this manually, with a fix for |
|
Oh, good eye! That's just been waiting to explode. Seeing you didn't yet create the commit, I'll fix this up. |
|
Oddly, fixing that makes a unit test fail on Windows...! I'm digging in now. |
|
But feel free to push a change to this branch - I'm curious if AppVeyor sees the same thing or if it's something bizarre on my machine. |
db42831 to
643189a
Compare
|
Oh, strange. Didn't get any errors on Linux. |
643189a to
a693b87
Compare
The function `git_buf_try_grow` consistently calls `giterr_set_oom` whenever growing the buffer fails due to insufficient memory being available. So in fact, we do not have to do this ourselves when a call to any buffer-growing function has failed due to an OOM situation. But we still do so in two functions, which this patch cleans up.
While the `ENSURE_SIZE` macro gets a reference to both the buffer that is to be resized and a new size, we were not consistently referencing the passed buffer, but instead a variable `buf`, which is not passed in. Funnily enough, we never noticed because our buffers seem to always be named `buf` whenever the macro was being used. Fix the macro by always using the passed-in buffer. While at it, add braces around all mentions of passed-in variables as should be done with macros to avoid subtle errors. Found-by: Edward Thompson
The `ENSURE_SIZE` macro can be used to grow a buffer if its currently allocated size does not suffice a required target size. While most of the code already uses this macro, the `git_buf_join` and `git_buf_join3` functions do not yet use it. Due to the macro first checking whether we have to grow the buffer at all, this has the benefit of saving a function call when it is not needed. While this is nice to have, it will probably not matter at all performance-wise -- instead, this only serves for consistency across the code.
Both the `git_buf_init` and `git_buf_attach` functions may call `git_buf_grow` in case they were given an allocation length as parameter. As such, it is possible for these functions to fail when we run out of memory. While it won't probably be used anytime soon, it does indeed make sense to also record this fact by returning an error code from both functions. As they belong to the internal API only, this change does not break our interface.
The `git_buf_init` function has an optional length parameter, which will cause the buffer to be initialized and allocated in one step. This can be used instead of static initialization with `GIT_BUF_INIT` followed by a `git_buf_grow`. This patch does so for two functions where it is applicable.
|
Taking a look at all calls to |
|
Yeah... presumably something else entirely, but the |
|
Failing for me in |
Small-ish cleanups for growing buffers. While this is mostly done to silence Coverity, I think it even improves code by a tiny fraction.