631155 - undefined return value in function '_cairo_surface_wrapper_flush'

Status: RESOLVED FIXED

Description

[Wolfgang Rosenauer [:wolfiR]]

A patch from bug 612846 introduced the new function _cairo_surface_wrapper_flush which is just:
cairo_status_t
_cairo_surface_wrapper_flush (cairo_surface_wrapper_t *wrapper)
{
    if (wrapper->target->backend->flush) {
	return wrapper->target->backend->flush(wrapper->target);
    }
}

gcc is rightfully complaining:
/usr/src/packages/BUILD/mozilla/gfx/cairo/cairo/src/cairo-surface-wrapper.c: In function '_cairo_surface_wrapper_flush':
/usr/src/packages/BUILD/mozilla/gfx/cairo/cairo/src/cairo-surface-wrapper.c:534:1: warning: control reaches end of non-void function
cairo-tee-surface.c

The function must return a correct cairo_status_t in every case.

Comment 1

[Ed Morley [:emorley]]

I presume you mean just adding something like:
|return CAIRO_STATUS_ZZZZZZZ;|
...after the if block here:
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-surface-wrapper.c#719

What status code should it be returning out of the list here?
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo.h#289

Presumably one of the error codes since |if (wrapper->target->backend->flush)| would have failed?

Thanks!

Comment 2

[Ed Morley [:emorley]]

Jeff, could you take a look at comment 1 for me please.

Thanks! :-)

Comment 3

[Steve Snyder]

Ping.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

I think returning CAIRO_STATUS_SUCCESS makes the most sense here. If we don't have a flush method we don't need to worry about flushing.

Comment 5

[Ed Morley [:emorley]]

Created attachment 544555 [details] [diff] [review]
Patch v1

Makes sense to me :-)

Removes tab whilst I'm there.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 544555 [details] [diff] [review]
Patch v1

Not sure if the indentation change is intentional. 

Please also include the patch in the cairo directory and update the README there.

Comment 7

[Ed Morley [:emorley]]

Created attachment 544766 [details] [diff] [review]
Patch v1.1

The indentation change was intentional (to make it match the lines before/after), however, I've reverted it now to reduce the number of upstream lines changed.

Only other changes are the addition of the patch to the cairo patches collection & the mention of it in the README there.

Carrying forwards r+.

Comment 8

[Ed Morley [:emorley]]

Passed try:
http://dev.philringnalda.com/tbpl/?tree=Try&rev=7e2dec83b318

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a02ef4cdf6b2

Comment 10

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/a02ef4cdf6b2