This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Remove CHECK_TYPEDEF, use check_typedef instead
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: Pedro Alves <palves at redhat dot com>, <gdb-patches at sourceware dot org>
- Date: Tue, 7 Jul 2015 16:15:05 -0400
- Subject: Re: [PATCH] Remove CHECK_TYPEDEF, use check_typedef instead
- Authentication-results: sourceware.org; auth=none
- References: <1436213157-21480-1-git-send-email-simon dot marchi at ericsson dot com> <559BFB12 dot 6050606 at redhat dot 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
>