On Thu, 9 Mar 2023, Darren Tucker wrote:
> On Thu, 9 Mar 2023 at 02:09, joshua stein <j...@jcs.org> wrote:
> > cppcheck found these, are they worth fixing?
> >
> > In the non-fail case, done is set to NULL and then free()d.
> > free(NULL) is legal but maybe worth removing?
>
> ssh uses this pattern a lot, and I agree with millert that it's not
> worth changing.
>
> char *thing = NULL;
> [lots of stuff that might set thing]
> if (maybe()) {
> free(thing);
> thing = something;
> }
> [more stuff]
> free(thing)
>
> We actually went through and changed all the "if (thing) free(thing)"
> instances to drop the "if" some time ago.
>
> > grp == NULL fatal()s, so remove the ternary operations that will
> > never be the conditionals they aspire to be.
>
> That's true in OpenBSD, but not in -portable where the check is a
> debug only. That said, there's already a diff in this code block
> that'll stop future syncs from applying cleanly so I don't mind either
> way.
>
> > + if ((r = encode_dest_constraint_hop(b, &dc->from)) != 0 ||
> > + (r = encode_dest_constraint_hop(b, &dc->to)) != 0 ||
>
> I'll wait for djm to comment on this one.
>
> > + if ((negate = (attrib[0] == '!')))
>
> This seems to be one too many parens? ie
> if (negate = (attrib[0] == '!'))
>
> > - if ((r = parse_dest_constraint_hop(frombuf, &dc->from) != 0) ||
> > - (r = parse_dest_constraint_hop(tobuf, &dc->to) != 0))
> > + if ((r = parse_dest_constraint_hop(frombuf, &dc->from)) != 0 ||
> > + (r = parse_dest_constraint_hop(tobuf, &dc->to)) != 0)
>
> also djm.
ok djm for these
