This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 02/11] [PR gdb/14441] gdb: gdbtypes: change {lookup,make}_reference_type() API
- From: Keith Seitz <keiths at redhat dot com>
- To: Artemiy Volkov <artemiyv at acm dot org>, gdb-patches at sourceware dot org
- Date: Wed, 16 Mar 2016 15:19:13 -0700
- Subject: Re: [PATCH v3 02/11] [PR gdb/14441] gdb: gdbtypes: change {lookup,make}_reference_type() API
- Authentication-results: sourceware.org; auth=none
- References: <1453229609-20159-1-git-send-email-artemiyv at acm dot org> <1457147955-21871-1-git-send-email-artemiyv at acm dot org> <1457147955-21871-3-git-send-email-artemiyv at acm dot org>
On 03/04/2016 07:19 PM, Artemiy Volkov wrote:
> Parameterize lookup_reference_type() and make_reference_type() by the kind of
> reference type we want to look up. Create two wrapper functions
> lookup_{lvalue,rvalue}_reference_type() for lookup_reference_type() to simplify
> the API. Change all callers to use the new API.
Just an FYI: When the time comes to commit this, GIT will require a
single-line description/summary of the patch, followed by a blank line.
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index f129b0e..a99e878 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -384,15 +384,21 @@ lookup_pointer_type (struct type *type)
> /* Lookup a C++ `reference' to a type TYPE. TYPEPTR, if nonzero,
> points to a pointer to memory where the reference type should be
> stored. If *TYPEPTR is zero, update it to point to the reference
> - type we return. We allocate new memory if needed. */
> + type we return. We allocate new memory if needed. REFCODE denotes
Need two spaces after "if needed."
> + the kind of reference type to lookup (lvalue or rvalue reference). */
>
> struct type *
> -make_reference_type (struct type *type, struct type **typeptr)
> +make_reference_type (struct type *type, struct type **typeptr,
> + enum type_code refcode)
> {
> struct type *ntype; /* New type */
> + struct type **reftype;
> struct type *chain;
>
> - ntype = TYPE_REFERENCE_TYPE (type);
> + gdb_assert (refcode == TYPE_CODE_REF || refcode == TYPE_CODE_RVALUE_REF);
> +
> + ntype = (refcode == TYPE_CODE_REF ? TYPE_REFERENCE_TYPE (type)
> + : TYPE_RVALUE_REFERENCE_TYPE (type));
>
> if (ntype)
> {
> @@ -421,7 +427,11 @@ make_reference_type (struct type *type, struct type **typeptr)
> }
>
> TYPE_TARGET_TYPE (ntype) = type;
> - TYPE_REFERENCE_TYPE (type) = ntype;
> + reftype = (refcode == TYPE_CODE_REF ? &TYPE_REFERENCE_TYPE (type)
> + : &TYPE_RVALUE_REFERENCE_TYPE (type));
> +
> + if(*reftype != NULL)
> + *reftype = ntype;
>
> /* FIXME! Assume the machine has only one representation for
> references, and that it matches the (only) representation for
I would prefer to do:
if (refcode == TYPE_CODE_REF)
TYPE_REFERENCE_TYPE (type) = ntype;
else
TYPE_RVALUE_REFERENCE_TYPE (type) = ntype;
It's cleaner/smaller/easier to read (at least for me). [Otherwise, a
space is missing between "if" and "(".] See next comment, though.
> @@ -429,10 +439,9 @@ make_reference_type (struct type *type, struct type **typeptr)
>
> TYPE_LENGTH (ntype) =
> gdbarch_ptr_bit (get_type_arch (type)) / TARGET_CHAR_BIT;
> - TYPE_CODE (ntype) = TYPE_CODE_REF;
> + TYPE_CODE (ntype) = refcode;
>
> - if (!TYPE_REFERENCE_TYPE (type)) /* Remember it, if don't have one. */
> - TYPE_REFERENCE_TYPE (type) = ntype;
> + *reftype = ntype;
>
This is slightly different from the original, which only set the new
type if it was unset in the type. Your revised version will
unconditionally do it every time. I have no idea if it makes a
difference or not. Do you?
> /* Update the length of all the other variants of this type. */
> chain = TYPE_CHAIN (ntype);
Keith