On Sep 4, 2015 1:48 AM, "Bryce Harrington" <<a href="mailto:bryce@osg.samsung.com">bryce@osg.samsung.com</a>> wrote: > > On Fri, Sep 04, 2015 at 12:17:18AM +0530, Seedo Eldho Paul wrote: > > xzalloc is guaranteed to return a non-NULL value. > > > > Signed-off-by: Seedo Eldho Paul <<a href="mailto:seedoeldhopaul@gmail.com">seedoeldhopaul@gmail.com</a>> > > --- > > tests/internal-screenshot-test.c | 13 ------------- > > 1 file changed, 13 deletions(-) > > Good point. The error handling code will never actually get executed. > > > diff --git a/tests/internal-screenshot-test.c b/tests/internal-screenshot-test.c > > index e72a695..a74ae7a 100644 > > --- a/tests/internal-screenshot-test.c > > +++ b/tests/internal-screenshot-test.c > > @@ -93,11 +93,6 @@ load_surface_from_png(const char *fname) > > > > /* Disguise the cairo surface in a weston test surface */ > > reference = xzalloc(sizeof *reference); > > - if (reference == NULL) { > > - perror("xzalloc reference"); > > - cairo_surface_destroy(reference_cairo_surface); > > - return NULL; > > - } > > This will skip the surface destruction. I suppose who cares, we're > exiting anyway, but the xzalloc could be moved up prior to the surface > creation, and make it a non-issue. > > > reference->width = cairo_image_surface_get_width(reference_cairo_surface); > > reference->height = cairo_image_surface_get_height(reference_cairo_surface); > > stride = cairo_image_surface_get_stride(reference_cairo_surface); > > @@ -115,12 +110,6 @@ load_surface_from_png(const char *fname) > > /* Allocate new buffer for our weston reference, and copy the data from > > the cairo surface so we can destroy it */ > > reference->data = xzalloc(source_data_size); > > - if (reference->data == NULL) { > > - perror("xzalloc reference data"); > > - cairo_surface_destroy(reference_cairo_surface); > > - free(reference); > > - return NULL; > > - } > > Can't do that here though, since we depend on the surface to calculate > source_data_size. Also freeing reference in this case is a good idea. > Maybe this is a case where we should plain zalloc instead? > > > memcpy(reference->data, > > cairo_image_surface_get_data(reference_cairo_surface), > > source_data_size); > > @@ -144,8 +133,6 @@ create_screenshot_surface(struct client *client) > > { > > struct surface* screenshot; > > screenshot = xzalloc(sizeof *screenshot); > > - if (screenshot == NULL) > > - return NULL; > > screenshot->wl_buffer = create_shm_buffer(client, > > client->output->width, > > client->output->height, > > Same as above, maybe should use zalloc instead. > > Thanks for reviewing this. Sorry if these comments seem nit-picky for a > just a test case... The reason is that I plan to move a lot of this > code out of this test into the testing infrastructure, so having it > handle errors cleanly in this test case will be worthwhile in the longer > run. > Thanks for the review. I agree with your rationale. I'll respin the patch later today. SEEDO > Bryce > > > -- > > 1.9.1 > > > > _______________________________________________ > > wayland-devel mailing list > > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a>