On Tue, Mar 04, 2014 at 09:21:50AM -0800, Christopher Li wrote:
> On Sat, Mar 1, 2014 at 3:41 AM, John Keeping <john@xxxxxxxxxxxxx> wrote:
> > This stops warnings in code using socket operations with a modern glibc,
> > which otherwise result in warnings of the form:
> >
> > warning: incorrect type in argument 2 (invalid types)
> > expected union __CONST_SOCKADDR_ARG [usertype] __addr
> > got struct sockaddr *<noident>
> >
> > Since transparent unions are only applicable to function arguments, we
> > create a new function to check that the types are compatible
> > specifically in this context.
>
> Can you please keep the option to warning about the transparent union?
> You can change the default to off. While you are there, please make the second
> patch base on the chrisl sparse repository.
Will do.
> > +static int compatible_argument_type(struct expression *expr, struct symbol *target,
> > + struct expression **rp, const char *where)
> > +{
> > + const char *typediff;
> > + struct symbol *source = degenerate(*rp);
> > + struct symbol *t;
> > + classify_type(target, &t);
> > +
> > + if (t->type == SYM_UNION && t->transparent_union) {
> > + struct symbol *member;
> > + FOR_EACH_PTR(t->symbol_list, member) {
> > + if (check_assignment_types(member, rp, &typediff))
> > + return 1;
> > + } END_FOR_EACH_PTR(member);
> > + }
> > +
> > + if (check_assignment_types(target, rp, &typediff))
> > + return 1;
> > +
> > + warning(expr->pos, "incorrect type in %s (%s)", where, typediff);
> > + info(expr->pos, " expected %s", show_typename(target));
> > + info(expr->pos, " got %s", show_typename(source));
> > + *rp = cast_to(*rp, target);
> > + return 0;
> > +}
>
> I found this compatible_argument_type() hard to read. There are code
> copy/paste from the function compatible_assignment_types().
>
> I think a better way is:
> static int compatible_argument_type(...)
> {
> struct symbol *t;
> classify_type(target, &t);
>
> if (t->type == SYM_UNION && t->transparent_union)
> return compatible_assignment_transparent_union(...);
> return compatible_assignment_types(...);
> }
>
> Then you just need to complete the function
> compatible_assignment_transparent_union().
> Call compatible_assignment_types() if needed.
I'm not sure I understand what you mean here. If I extract
compatible_assignment_transparent_union() then it is essentially the
same as compatible_argument_type() but without the check for
t->transparent_union.
Looking again, I can see that my implementation above is unnecessarily
complicated because the warning() block is identical to that in
compatible_assignment_types() and there's no way for typediff to escape
from the transparent_union look, so the last 8 lines can be replaced by:
return compatible_assignment_types(target, rp, &typediff);
That also allows us to get rid of 'source', so we end up with:
static int compatible_argument_type(struct expression *expr, struct symbol *target,
struct expression **rp, const char *where)
{
struct symbol *t;
classify_type(target, &t);
if (t->type == SYM_UNION && t->transparent_union) {
const char *typediff;
struct symbol *member;
FOR_EACH_PTR(t->symbol_list, member) {
if (check_assignment_types(member, rp, &typediff))
return 1;
} END_FOR_EACH_PTR(member);
}
return compatible_assignment_types(expr, target, rp, where);
}
I'm not sure moving the contents of the if block into a separate
function improves things much at that point. What do you think?
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
