On Tue, Mar 4, 2014 at 9:52 AM, John Keeping <john@xxxxxxxxxxxxx> wrote:
> 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?
That is better, if you submit code like that, I will not reject it.
The difference is relatively minor, fall into the personal preference category.
Let me explain why I suggest that way.
The if statement for transparent union is a very rare and special case.
In other world, it is the cold and slow path.
People reading the compatible_argument_type() functions will most likely
have the normal assignment check in mind. It will go straight to the last
return statement. That is the hot and fast path.
And the implementation of transparent union aka the slow path is a lot
more complicate than the fast path. It is justifiable to move them into a
separate function. It also help the compiler inline compatible_argument_type()
if the slow path end up too big and complicate to inline.
>From the code reading point of view, I found it helps to clearly separate the
simple hot path and the complicate slow path.
Again, it is your call. I will not enforce it.
Chris
--
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
