Re: [PATCH] Remove CHECK_TYPEDEF, use check_typedef instead

[Simon Marchi <simon.marchi@ericsson.com>]

On 15-07-07 12:15 PM, Pedro Alves wrote:
> I don't have a strong opinion either.  Whatever's fine with others
> is fine with me.
> 
> Playing devil's advocate, the CHECK_TYPEDEF macro has the advantage
> that makes it clear that you want to peel away typedefs are don't
> really care about the original type.

I think it's clear enough as well when you assign to the same variable
as the one you pass.

> I'd argue that the real issue with the macro is that it takes the
> type pointer argument "by non-const reference to pointer".
> 
> Another solution would be to make it a function/macro that
> instead takes a pointer to a type pointer.  Something like:
> 
>  void
>  CHECK_TYPEDEFS (struct type **type)
>  {
>    *type = check_typedef (*type);
>  }
> 
> Then you'd write:
> 
>  > -	  CHECK_TYPEDEF (result);
>  > +	  CHECK_TYPEDEF (&result);
> 
> Or even rename it while at it:
> 
>  void
>  peel_typedefs (struct type **type)
>  {
>    *type = check_typedef (*type);
>  }
> 
> And so you'd write:
> 
>  > -	  CHECK_TYPEDEF (result);
>  > +	  peel_typedefs (&result);
> 
> Then the code ends up self documenting, and there's no way to
> forget to assign the return of the function back to the
> argument.

That's a bit better, but I don't think there's an advantage here of
having two ways to do the same thing.  If anything, it's confusing for
new contributors.

I was going to suggest adding __attribute__((warn_unused_result)) to
check_typedef, as it would also prevent forgetting assigning the result.
Especially if we change it, people used to the old macro would be at
risk of forgetting it.  However, I realized that at a few places the
result of check_typedef is ignored.  It is used only to initialize the
length field of the typedef, so that TYPE_LENGTH (the_typedef) will
return the right thing [1].

Using a side-effect of check_typedef to get the length of the type right
seems very hackish and error-prone (easy to forget something or to break
something when moving code around).

I think there should be a get_type_length [2] function that returns what
you would expect: the actual length of the type, after having peeled all
layers of typedef. You wouldn't need to call check_typedef beforehand.
It would be cleaner and safer, and would allow us to add warn_unused_result
to check_typedef.  Because there should be no reason to call check_typedef
than to obtain the resolved type.

Thoughts?

[1] For example: https://github.com/simark/binutils-gdb/blob/master/gdb/tracepoint.c#L1532
[2] I can see a get_type_length function mentioned in the ChangeLog here:

      https://github.com/simark/binutils-gdb/blob/master/gdb/ChangeLog-2014#L9750

    but I can't find any trace of it in the source code (even when checkout out
    that commit). Any idea? Joel perhaps?

> Thanks,
> Pedro Alves
>