>
> 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
>
This is going OT, I already updated the patch.
About checking for pointer assuming you have to check for NULL
would mean the NULL is not 0 which would mean that
memset(structure_ptr, 0, sizeof(structure)) cannot set pointer
to NULL which would mean that assuming pointer initialized from
GObject NULL are not correct, I would personally vote again
the if (pointer != NULL) style, if (pointer) is good as well.
About ?: I would limit, maybe if you can write it into a single
line, not more, it's a C syntax since ever.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel