[Spice-devel] server: Remove unneeded asserts in mjpeg_encoder_get_stats()

Submitted by Francois Gouget on Nov. 23, 2015, 3:25 p.m.

Details

Message ID alpine.DEB.2.20.1511231623350.9799@amboise
State New
Headers show
Series "server: Remove unneeded asserts in mjpeg_encoder_get_stats()" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Francois Gouget Nov. 23, 2015, 3:25 p.m.
No other function checks the 'this' pointer and if not given a buffer
to return the statistics we can simply not return any.

Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
---
 server/mjpeg_encoder.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

So following the logging discussion it looks like we would be going 
towards using the g_return_xxx() functions so in 
mjpeg_encoder_get_bit_rate()'s case we would get something like this. 
Makes sense?

Patch hide | download patch | download mbox

diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
index e5f3cbd..8998462 100644
--- a/server/mjpeg_encoder.c
+++ b/server/mjpeg_encoder.c
@@ -25,6 +25,7 @@ 
 #include <jerror.h>
 #include <jpeglib.h>
 #include <inttypes.h>
+#include <glib.h>
 
 #define MJPEG_MAX_FPS 25
 #define MJPEG_MIN_FPS 1
@@ -1330,7 +1331,7 @@  uint64_t mjpeg_encoder_get_bit_rate(MJpegEncoder *encoder)
 
 void mjpeg_encoder_get_stats(MJpegEncoder *encoder, MJpegEncoderStats *stats)
 {
-    spice_assert(encoder != NULL && stats != NULL);
+    g_return_if_fail(stats != NULL);
     stats->starting_bit_rate = encoder->starting_bit_rate;
     stats->cur_bit_rate = mjpeg_encoder_get_bit_rate(encoder);
     stats->avg_quality = (double)encoder->avg_quality / encoder->num_frames;

Comments

> 
> No other function checks the 'this' pointer and if not given a buffer
> to return the statistics we can simply not return any.
> 
> Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> ---
>  server/mjpeg_encoder.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> So following the logging discussion it looks like we would be going
> towards using the g_return_xxx() functions so in
> mjpeg_encoder_get_bit_rate()'s case we would get something like this.
> Makes sense?
> 

Ehm... no, NACK

Frediano

> diff --git a/server/mjpeg_encoder.c b/server/mjpeg_encoder.c
> index e5f3cbd..8998462 100644
> --- a/server/mjpeg_encoder.c
> +++ b/server/mjpeg_encoder.c
> @@ -25,6 +25,7 @@
>  #include <jerror.h>
>  #include <jpeglib.h>
>  #include <inttypes.h>
> +#include <glib.h>
>  
>  #define MJPEG_MAX_FPS 25
>  #define MJPEG_MIN_FPS 1
> @@ -1330,7 +1331,7 @@ uint64_t mjpeg_encoder_get_bit_rate(MJpegEncoder
> *encoder)
>  
>  void mjpeg_encoder_get_stats(MJpegEncoder *encoder, MJpegEncoderStats
>  *stats)
>  {
> -    spice_assert(encoder != NULL && stats != NULL);
> +    g_return_if_fail(stats != NULL);
>      stats->starting_bit_rate = encoder->starting_bit_rate;
>      stats->cur_bit_rate = mjpeg_encoder_get_bit_rate(encoder);
>      stats->avg_quality = (double)encoder->avg_quality / encoder->num_frames;
> --
> 2.6.2
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
On Mon, Nov 23, 2015 at 12:07:49PM -0500, Frediano Ziglio wrote:
> > 
> > No other function checks the 'this' pointer and if not given a buffer
> > to return the statistics we can simply not return any.
> > 
> > Signed-off-by: Francois Gouget <fgouget@codeweavers.com>
> > ---
> >  server/mjpeg_encoder.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > So following the logging discussion it looks like we would be going
> > towards using the g_return_xxx() functions so in
> > mjpeg_encoder_get_bit_rate()'s case we would get something like this.
> > Makes sense?
> > 
> 
> Ehm... no, NACK

Some rationale is needed so that Francois knows how to improve his
patch. Is the assert->return_if_fail change wrong and unwanted? Should
he use some other variant of return_if_fail?

Christophe