[Spice-devel] [PATCH spice-server 2/3] test-gst: Remove options parsing leaks

[Christophe Fergeau]

On Tue, Sep 12, 2017 at 05:44:23AM -0400, Frediano Ziglio wrote:
> > 
> > On Tue, Sep 12, 2017 at 04:11:57AM -0400, Frediano Ziglio wrote:
> > > > 
> > > > On Tue, Sep 12, 2017 at 03:41:15AM -0400, Frediano Ziglio wrote:
> > > > > > 
> > > > > > On Tue, Sep 12, 2017 at 03:19:12AM -0400, Frediano Ziglio wrote:
> > > > > > > > > -    const EncoderInfo *encoder =
> > > > > > > > > get_encoder_info(encoder_name);
> > > > > > > > > +    const EncoderInfo *encoder = get_encoder_info(encoder_name
> > > > > > > > > ?
> > > > > > > > > encoder_name : "mjpeg");
> > > > > > > > >      if (!encoder) {
> > > > > > > > >          g_printerr("Encoder name unsupported: %s\n",
> > > > > > > > >          encoder_name);
> > > > > > > > 
> > > > > > > > This is going to be "Encoder name unsupported: (null)" when the
> > > > > > > > corresponding option is not given, ditto for the other options
> > > > > > > > that
> > > > > > > > you
> > > > > > > > changed.
> > > > > > > > 
> > > > > > > 
> > > > > > > No, the defaults are present so encoder won't be NULL... but I can
> > > > > > > change
> > > > > > > the code to make this more clear.
> > > > > > 
> > > > > > I'm missing something then, the code I quoted is:
> > > > > > 
> > > > > >      const EncoderInfo *encoder = get_encoder_info(encoder_name ?
> > > > > >      encoder_name : "mjpeg");
> > > > > >      if (!encoder) {
> > > > > >           g_printerr("Encoder name unsupported: %s\n", encoder_name);
> > > > > > 
> > > > > > so it looks like encoder_name can be NULL in the call to
> > > > > > get_encoder_info(), in which case we call get_encoder_info("mjpeg")
> > > > > > instead of get_encoder_info(encoder_name), but right after that, in
> > > > > > the
> > > > > > g_printerr call, encoder_name cannot be NULL?
> > > > > > 
> > > > > 
> > > > > No, get_encoder_info("mjpeg") is not supposed to return NULL... but I
> > > > > changed
> > > > > the code to understand it even without reading get_encoder_info,
> > > > > better.
> > > > 
> > > > But you are printing "encoder_name" (the input parameter), not "encoder"
> > > > (the
> > > > output) !
> > > > 
> > > 
> > > Yes, the name is encoder_name, the print is referring to name.
> > > By the way, changed code in last version I sent, should be clear, now is
> > > 
> > >     const EncoderInfo *encoder =
> > >         encoder_name ? get_encoder_info(encoder_name) : encoder_info_mjpeg;
> > >     if (!encoder) {
> > >         g_printerr("Encoder name unsupported: %s\n", encoder_name);
> > >         exit(1);
> > >     }
> > 
> > Which is still awful to read..
> > 
> 
> This is a personal opinion, I don't see any opposite advice in the style
> document, not any esoteric feature used

Yes this is a personal opinion, but the easier the code is to read for
the reviewer, the faster your code is going to get in.
I'm totally fine with updating the style document to say that pointer
checks should be (encoder != NULL), and to limit (!encoder) to booleans.
I'm also fine discouraging ?: use.

Christophe