[Mesa-dev] [PATCH] i965: Use new/delete instead of realloc() in brw_ir_allocator.h

[Kenneth Graunke]

On Wednesday, February 11, 2015 04:56:56 PM Juha-Pekka Heikkila wrote:
> There is no error path available thus instead of giving
> realloc possibility to fail use new which will never
> return null pointer and throws bad_alloc on failure.
> 
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> ---
>  src/mesa/drivers/dri/i965/brw_ir_allocator.h | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_ir_allocator.h b/src/mesa/drivers/dri/i965/brw_ir_allocator.h
> index b1237ed..5330bff 100644
> --- a/src/mesa/drivers/dri/i965/brw_ir_allocator.h
> +++ b/src/mesa/drivers/dri/i965/brw_ir_allocator.h
> @@ -40,8 +40,8 @@ namespace brw {
>  
>        ~simple_allocator()
>        {
> -         free(offsets);
> -         free(sizes);
> +         delete[] offsets;
> +         delete[] sizes;
>        }
>  
>        unsigned
> @@ -49,8 +49,16 @@ namespace brw {
>        {
>           if (capacity <= count) {
>              capacity = MAX2(16, capacity * 2);
> -            sizes = (unsigned *)realloc(sizes, capacity * sizeof(unsigned));
> -            offsets = (unsigned *)realloc(offsets, capacity * sizeof(unsigned));
> +
> +            unsigned *tmp_sizes = new unsigned[capacity];
> +            memcpy(tmp_sizes, sizes, count * sizeof(unsigned));
> +            delete[] sizes;
> +            sizes = tmp_sizes;
> +
> +            unsigned *tmp_offsets = new unsigned[capacity];
> +            memcpy(tmp_offsets, offsets, count * sizeof(unsigned));
> +            delete[] offsets;
> +            offsets = tmp_offsets;
>           }
>  
>           sizes[count] = size;
> 

Okay, I'll go ahead and say what Matt and Jason were probably thinking:

NAK.

I'm not against using new/delete in general, but using realloc to double
the size of arrays is a pattern we use all over the codebase.  I don't
see any reason not to use that same pattern here.  Plus, the old code
was 2 lines of code...and now it's 8 lines of code, for no real benefit.

Most idiomatic C++ code I've seen would just some kind of flexible array
class, like std::vector, rather than open coding this.  So, we're
already divergent from C++ best practices, and instead are following the
typical C idiom.  This is neither.

In fact, your new code is more likely to fail than the old one: instead
of growing the array from size N to size 2*N---which realloc may be able
to do in-place---you're briefly keeping both around, using N+2*N=3*N
space.  If realloc failed, this would absolutely fail too.

If we compiled without exceptions, this would crash in an identical
manner - no actual error handling is done here.  With exceptions, it's
not much better - we throw an exception all the way outside of libGL and
back to the application (since we sure don't handle those), leaving our
work in some unknown state.

Plus, as has been repeatedly mentioned - malloc doesn't fail if you're
out of memory - it only fails if you're out of virtual address space.

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150211/7b18eee4/attachment.sig>