[commit][obv] Use TYPE_LENGTH directly where possible

[commit][obv] Use TYPE_LENGTH directly where possible

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

Hi,

I have committed[1] more changes that result in using TYPE_LENGTH
directly instead of using a local variable.  In amd64-tdep and
bfin-tdep, I use this in the gdb_assert so that the LEN data type then
does not need to be changed.  Other cases are the usual elimination of
the variable to use TYPE_LENGTH directly.

This is to reduce the size of the bitpos expansion patch[2].

Regards,
Siddhesh

[1] http://sourceware.org/ml/gdb-cvs/2012-09/msg00147.html
[2] http://sourceware.org/ml/gdb-patches/2012-08/msg00144.html

        * amd64-tdep.c (amd64_return_value): Use TYPE_LENGTH directly.
        * bfin-tdep.c (bfin_extract_return_value): Likewise.
        (bfin_store_return_value): Likewise.
        * cris-tdep.c (cris_store_return_value): Likewise.
        (cris_extract_return_value): Likewise.
        * h8300-tdep.c (h8300_extract_return_value): Likewise.
        * hppa-tdep.c (hppa64_return_value): Likewise.
        * lm32-tdep.c (lm32_store_return_value): Likewise.
        * microblaze-tdep.c (microblaze_store_return_value): Likewise.
        * spu-tdep.c (spu_value_from_register): Likewise.
        * vax-tdep.c (vax_return_value): Likewise.


[-- Attachment #2: trivial.patch --]
[-- Type: text/x-patch, Size: 10392 bytes --]

Index: gdb/amd64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/amd64-tdep.c,v
retrieving revision 1.110
diff -u -p -r1.110 amd64-tdep.c
--- gdb/amd64-tdep.c	25 Sep 2012 12:48:52 -0000	1.110
+++ gdb/amd64-tdep.c	26 Sep 2012 07:45:39 -0000
@@ -637,7 +637,7 @@ amd64_return_value (struct gdbarch *gdba
     }
 
   gdb_assert (class[1] != AMD64_MEMORY);
-  gdb_assert (len <= 16);
+  gdb_assert (TYPE_LENGTH (type) <= 16);
 
   for (i = 0; len > 0; i++, len -= 8)
     {
Index: gdb/bfin-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/bfin-tdep.c,v
retrieving revision 1.11
diff -u -p -r1.11 bfin-tdep.c
--- gdb/bfin-tdep.c	25 Sep 2012 12:48:52 -0000	1.11
+++ gdb/bfin-tdep.c	26 Sep 2012 07:45:39 -0000
@@ -615,7 +615,7 @@ bfin_extract_return_value (struct type *
   ULONGEST tmp;
   int regno = BFIN_R0_REGNUM;
 
-  gdb_assert (len <= 8);
+  gdb_assert (TYPE_LENGTH (type) <= 8);
 
   while (len > 0)
     {
@@ -643,7 +643,7 @@ bfin_store_return_value (struct type *ty
   int len = TYPE_LENGTH (type);
   int regno = BFIN_R0_REGNUM;
 
-  gdb_assert (len <= 8);
+  gdb_assert (TYPE_LENGTH (type) <= 8);
 
   while (len > 0)
     {
Index: gdb/cris-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/cris-tdep.c,v
retrieving revision 1.185
diff -u -p -r1.185 cris-tdep.c
--- gdb/cris-tdep.c	18 May 2012 21:02:47 -0000	1.185
+++ gdb/cris-tdep.c	26 Sep 2012 07:45:40 -0000
@@ -1662,20 +1662,20 @@ cris_store_return_value (struct type *ty
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   ULONGEST val;
-  int len = TYPE_LENGTH (type);
   
-  if (len <= 4)
+  if (TYPE_LENGTH (type) <= 4)
     {
       /* Put the return value in R10.  */
-      val = extract_unsigned_integer (valbuf, len, byte_order);
+      val = extract_unsigned_integer (valbuf, TYPE_LENGTH (type), byte_order);
       regcache_cooked_write_unsigned (regcache, ARG1_REGNUM, val);
     }
-  else if (len <= 8)
+  else if (TYPE_LENGTH (type) <= 8)
     {
       /* Put the return value in R10 and R11.  */
       val = extract_unsigned_integer (valbuf, 4, byte_order);
       regcache_cooked_write_unsigned (regcache, ARG1_REGNUM, val);
-      val = extract_unsigned_integer ((char *)valbuf + 4, len - 4, byte_order);
+      val = extract_unsigned_integer ((char *)valbuf + 4,
+				      TYPE_LENGTH (type) - 4, byte_order);
       regcache_cooked_write_unsigned (regcache, ARG2_REGNUM, val);
     }
   else
@@ -1833,21 +1833,21 @@ cris_extract_return_value (struct type *
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   ULONGEST val;
-  int len = TYPE_LENGTH (type);
   
-  if (len <= 4)
+  if (TYPE_LENGTH (type) <= 4)
     {
       /* Get the return value from R10.  */
       regcache_cooked_read_unsigned (regcache, ARG1_REGNUM, &val);
-      store_unsigned_integer (valbuf, len, byte_order, val);
+      store_unsigned_integer (valbuf, TYPE_LENGTH (type), byte_order, val);
     }
-  else if (len <= 8)
+  else if (TYPE_LENGTH (type) <= 8)
     {
       /* Get the return value from R10 and R11.  */
       regcache_cooked_read_unsigned (regcache, ARG1_REGNUM, &val);
       store_unsigned_integer (valbuf, 4, byte_order, val);
       regcache_cooked_read_unsigned (regcache, ARG2_REGNUM, &val);
-      store_unsigned_integer ((char *)valbuf + 4, len - 4, byte_order, val);
+      store_unsigned_integer ((char *)valbuf + 4, TYPE_LENGTH (type) - 4,
+			      byte_order, val);
     }
   else
     error (_("cris_extract_return_value: type length too large"));
Index: gdb/h8300-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/h8300-tdep.c,v
retrieving revision 1.136
diff -u -p -r1.136 h8300-tdep.c
--- gdb/h8300-tdep.c	25 Sep 2012 12:48:53 -0000	1.136
+++ gdb/h8300-tdep.c	26 Sep 2012 07:45:40 -0000
@@ -751,12 +751,12 @@ h8300_extract_return_value (struct type 
   int len = TYPE_LENGTH (type);
   ULONGEST c, addr;
 
-  switch (len)
+  switch (TYPE_LENGTH (type))
     {
     case 1:
     case 2:
       regcache_cooked_read_unsigned (regcache, E_RET0_REGNUM, &c);
-      store_unsigned_integer (valbuf, len, byte_order, c);
+      store_unsigned_integer (valbuf, TYPE_LENGTH (type), byte_order, c);
       break;
     case 4:			/* Needs two registers on plain H8/300 */
       regcache_cooked_read_unsigned (regcache, E_RET0_REGNUM, &c);
@@ -768,8 +768,9 @@ h8300_extract_return_value (struct type 
       if (TYPE_CODE (type) == TYPE_CODE_INT)
 	{
 	  regcache_cooked_read_unsigned (regcache, E_RET0_REGNUM, &addr);
-	  c = read_memory_unsigned_integer ((CORE_ADDR) addr, len, byte_order);
-	  store_unsigned_integer (valbuf, len, byte_order, c);
+	  c = read_memory_unsigned_integer ((CORE_ADDR) addr,
+					    TYPE_LENGTH (type), byte_order);
+	  store_unsigned_integer (valbuf, TYPE_LENGTH (type), byte_order, c);
 	}
       else
 	{
Index: gdb/hppa-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/hppa-tdep.c,v
retrieving revision 1.281
diff -u -p -r1.281 hppa-tdep.c
--- gdb/hppa-tdep.c	18 May 2012 21:02:48 -0000	1.281
+++ gdb/hppa-tdep.c	26 Sep 2012 07:45:40 -0000
@@ -1160,7 +1160,7 @@ hppa64_return_value (struct gdbarch *gdb
   int len = TYPE_LENGTH (type);
   int regnum, offset;
 
-  if (len > 16)
+  if (TYPE_LENGTH (type) > 16)
     {
       /* All return values larget than 128 bits must be aggregate
          return values.  */
Index: gdb/lm32-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/lm32-tdep.c,v
retrieving revision 1.13
diff -u -p -r1.13 lm32-tdep.c
--- gdb/lm32-tdep.c	25 Sep 2012 12:48:53 -0000	1.13
+++ gdb/lm32-tdep.c	26 Sep 2012 07:45:41 -0000
@@ -349,18 +349,18 @@ lm32_store_return_value (struct type *ty
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   ULONGEST val;
-  int len = TYPE_LENGTH (type);
 
-  if (len <= 4)
+  if (TYPE_LENGTH (type) <= 4)
     {
-      val = extract_unsigned_integer (valbuf, len, byte_order);
+      val = extract_unsigned_integer (valbuf, TYPE_LENGTH (type), byte_order);
       regcache_cooked_write_unsigned (regcache, SIM_LM32_R1_REGNUM, val);
     }
-  else if (len <= 8)
+  else if (TYPE_LENGTH (type) <= 8)
     {
       val = extract_unsigned_integer (valbuf, 4, byte_order);
       regcache_cooked_write_unsigned (regcache, SIM_LM32_R1_REGNUM, val);
-      val = extract_unsigned_integer (valbuf + 4, len - 4, byte_order);
+      val = extract_unsigned_integer (valbuf + 4, TYPE_LENGTH (type) - 4,
+				      byte_order);
       regcache_cooked_write_unsigned (regcache, SIM_LM32_R2_REGNUM, val);
     }
   else
Index: gdb/microblaze-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/microblaze-tdep.c,v
retrieving revision 1.13
diff -u -p -r1.13 microblaze-tdep.c
--- gdb/microblaze-tdep.c	2 Aug 2012 09:36:39 -0000	1.13
+++ gdb/microblaze-tdep.c	26 Sep 2012 07:45:41 -0000
@@ -590,22 +590,21 @@ static void
 microblaze_store_return_value (struct type *type, struct regcache *regcache,
 			       const gdb_byte *valbuf)
 {
-  int len = TYPE_LENGTH (type);
   gdb_byte buf[8];
 
   memset (buf, 0, sizeof(buf));
 
   /* Integral and pointer return values.  */
 
-  if (len > 4)
+  if (TYPE_LENGTH (type) > 4)
     {
-       gdb_assert (len == 8);
+       gdb_assert (TYPE_LENGTH (type) == 8);
        memcpy (buf, valbuf, 8);
        regcache_cooked_write (regcache, MICROBLAZE_RETVAL_REGNUM+1, buf + 4);
     }
   else
     /* ??? Do we need to do any sign-extension here?  */
-    memcpy (buf + 4 - len, valbuf, len);
+    memcpy (buf + 4 - TYPE_LENGTH (type), valbuf, TYPE_LENGTH (type));
 
   regcache_cooked_write (regcache, MICROBLAZE_RETVAL_REGNUM, buf);
 }
Index: gdb/spu-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/spu-tdep.c,v
retrieving revision 1.82
diff -u -p -r1.82 spu-tdep.c
--- gdb/spu-tdep.c	17 Sep 2012 08:52:18 -0000	1.82
+++ gdb/spu-tdep.c	26 Sep 2012 07:45:41 -0000
@@ -316,11 +316,10 @@ spu_value_from_register (struct type *ty
 			 struct frame_info *frame)
 {
   struct value *value = default_value_from_register (type, regnum, frame);
-  int len = TYPE_LENGTH (type);
 
-  if (regnum < SPU_NUM_GPRS && len < 16)
+  if (regnum < SPU_NUM_GPRS && TYPE_LENGTH (type) < 16)
     {
-      int preferred_slot = len < 4 ? 4 - len : 0;
+      int preferred_slot = TYPE_LENGTH (type) < 4 ? 4 - TYPE_LENGTH (type) : 0;
       set_value_offset (value, preferred_slot);
     }
 
Index: gdb/vax-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/vax-tdep.c,v
retrieving revision 1.112
diff -u -p -r1.112 vax-tdep.c
--- gdb/vax-tdep.c	16 May 2012 14:35:08 -0000	1.112
+++ gdb/vax-tdep.c	26 Sep 2012 07:45:41 -0000
@@ -208,7 +208,6 @@ vax_return_value (struct gdbarch *gdbarc
 		  struct type *type, struct regcache *regcache,
 		  gdb_byte *readbuf, const gdb_byte *writebuf)
 {
-  int len = TYPE_LENGTH (type);
   gdb_byte buf[8];
 
   if (TYPE_CODE (type) == TYPE_CODE_STRUCT
@@ -224,7 +223,7 @@ vax_return_value (struct gdbarch *gdbarc
 	  ULONGEST addr;
 
 	  regcache_raw_read_unsigned (regcache, VAX_R0_REGNUM, &addr);
-	  read_memory (addr, readbuf, len);
+	  read_memory (addr, readbuf, TYPE_LENGTH (type));
 	}
 
       return RETURN_VALUE_ABI_RETURNS_ADDRESS;
@@ -234,16 +233,16 @@ vax_return_value (struct gdbarch *gdbarc
     {
       /* Read the contents of R0 and (if necessary) R1.  */
       regcache_cooked_read (regcache, VAX_R0_REGNUM, buf);
-      if (len > 4)
+      if (TYPE_LENGTH (type) > 4)
 	regcache_cooked_read (regcache, VAX_R1_REGNUM, buf + 4);
-      memcpy (readbuf, buf, len);
+      memcpy (readbuf, buf, TYPE_LENGTH (type));
     }
   if (writebuf)
     {
       /* Read the contents to R0 and (if necessary) R1.  */
-      memcpy (buf, writebuf, len);
+      memcpy (buf, writebuf, TYPE_LENGTH (type));
       regcache_cooked_write (regcache, VAX_R0_REGNUM, buf);
-      if (len > 4)
+      if (TYPE_LENGTH (type) > 4)
 	regcache_cooked_write (regcache, VAX_R1_REGNUM, buf + 4);
     }
 

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Joel Brobecker]

> @@ -637,7 +637,7 @@ amd64_return_value (struct gdbarch *gdba
>      }
>  
>    gdb_assert (class[1] != AMD64_MEMORY);
> -  gdb_assert (len <= 16);
> +  gdb_assert (TYPE_LENGTH (type) <= 16);
>  
>    for (i = 0; len > 0; i++, len -= 8)
>      {

Why is the type not OK for the assert, and yet OK for the rest of
the code? (the same question applies to other files, as well)

> Index: gdb/cris-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/cris-tdep.c,v
> retrieving revision 1.185
> diff -u -p -r1.185 cris-tdep.c
> --- gdb/cris-tdep.c	18 May 2012 21:02:47 -0000	1.185
> +++ gdb/cris-tdep.c	26 Sep 2012 07:45:40 -0000
> @@ -1662,20 +1662,20 @@ cris_store_return_value (struct type *ty
>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    ULONGEST val;
> -  int len = TYPE_LENGTH (type);
>    
> -  if (len <= 4)
> +  if (TYPE_LENGTH (type) <= 4)
>      {
>        /* Put the return value in R10.  */
> -      val = extract_unsigned_integer (valbuf, len, byte_order);
> +      val = extract_unsigned_integer (valbuf, TYPE_LENGTH (type), byte_order);
>        regcache_cooked_write_unsigned (regcache, ARG1_REGNUM, val);
>      }
> -  else if (len <= 8)
> +  else if (TYPE_LENGTH (type) <= 8)
>      {
>        /* Put the return value in R10 and R11.  */
>        val = extract_unsigned_integer (valbuf, 4, byte_order);
>        regcache_cooked_write_unsigned (regcache, ARG1_REGNUM, val);
> -      val = extract_unsigned_integer ((char *)valbuf + 4, len - 4, byte_order);
> +      val = extract_unsigned_integer ((char *)valbuf + 4,
> +				      TYPE_LENGTH (type) - 4, byte_order);
>        regcache_cooked_write_unsigned (regcache, ARG2_REGNUM, val);
>      }
>    else
> @@ -1833,21 +1833,21 @@ cris_extract_return_value (struct type *
>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    ULONGEST val;
> -  int len = TYPE_LENGTH (type);
>    
> -  if (len <= 4)
> +  if (TYPE_LENGTH (type) <= 4)
>      {
>        /* Get the return value from R10.  */
>        regcache_cooked_read_unsigned (regcache, ARG1_REGNUM, &val);
> -      store_unsigned_integer (valbuf, len, byte_order, val);
> +      store_unsigned_integer (valbuf, TYPE_LENGTH (type), byte_order, val);
>      }
> -  else if (len <= 8)
> +  else if (TYPE_LENGTH (type) <= 8)
>      {
>        /* Get the return value from R10 and R11.  */
>        regcache_cooked_read_unsigned (regcache, ARG1_REGNUM, &val);
>        store_unsigned_integer (valbuf, 4, byte_order, val);
>        regcache_cooked_read_unsigned (regcache, ARG2_REGNUM, &val);
> -      store_unsigned_integer ((char *)valbuf + 4, len - 4, byte_order, val);
> +      store_unsigned_integer ((char *)valbuf + 4, TYPE_LENGTH (type) - 4,
> +			      byte_order, val);
>      }
>    else
>      error (_("cris_extract_return_value: type length too large"));

Why is it better to repeat the use of TYPE_LENGTH rather than use
a single variable? It's definitely not obvious to me, and it seems
even simpler to just change the type of variable "len"... This patch
feels like a step backwards, and trying to reduce the size of a patch
would be the wrong justification for it.

-- 
Joel

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Siddhesh Poyarekar]

On Wed, 26 Sep 2012 11:42:27 +0200, Joel wrote:
> > @@ -637,7 +637,7 @@ amd64_return_value (struct gdbarch *gdba
> >      }
> >  
> >    gdb_assert (class[1] != AMD64_MEMORY);
> > -  gdb_assert (len <= 16);
> > +  gdb_assert (TYPE_LENGTH (type) <= 16);
> >  
> >    for (i = 0; len > 0; i++, len -= 8)
> >      {
> 
> Why is the type not OK for the assert, and yet OK for the rest of
> the code? (the same question applies to other files, as well)

This is so that the assert is not subject to any truncation/overflow
resulting due to the type of LEN.  That way, I don't have to expand LEN
since I know that the value is always going to be less than 16 and if
something actually goes wrong, then the assert will definitely catch it.

> Why is it better to repeat the use of TYPE_LENGTH rather than use
> a single variable? It's definitely not obvious to me, and it seems
> even simpler to just change the type of variable "len"... This patch
> feels like a step backwards, and trying to reduce the size of a patch
> would be the wrong justification for it.
> 

It does not make any functional difference at all and the justification
is in fact that it reduces the size of the bitpos patch.  I have
committed similar changes in the past that were deemed to be OK, so I
don't see why this patch in particular should be a problem.

I have not made changes in places where more than 4-5 substitutions
were necessary and where the code started looking unwieldy as a
result.  I guess both those parameters are subjective, but I couldn't
see a coding convention that seems to have been strictly followed
throughout the code base.

Regards,
Siddhesh

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Joel Brobecker]

> > Why is the type not OK for the assert, and yet OK for the rest of
> > the code? (the same question applies to other files, as well)
> 
> This is so that the assert is not subject to any truncation/overflow
> resulting due to the type of LEN.  That way, I don't have to expand LEN
> since I know that the value is always going to be less than 16 and if
> something actually goes wrong, then the assert will definitely catch it.

OK. I see why it works.

But I can definitely see someone like myself missing that subtlety,
and commit an obvious change that reduces the duplication by re-using
the variable in the gdb_assert call. To the unattentive me, that's
an obvious improvement. I think that's an issue.

> It does not make any functional difference at all and the justification
> is in fact that it reduces the size of the bitpos patch.  I have
> committed similar changes in the past that were deemed to be OK, so I
> don't see why this patch in particular should be a problem.

I know it makes no functional difference, but it does make a noticeable
difference in terms of maintenance, IMO. And I have in fact been
silently grumbling about your patches being labeled "obvious" when
in fact I do not consider them obvious. But I have let them go, because
it wasn't significant enough that I felt I needed to talk about it.

But this part of this patch in particular did catch my attention, and
I feel that we need to discuss it. I think this goes in the wrong
direction, and it would be better to just change the variable
(constant???) type, rather than duplicate the expression everywhere.
I don't have a strong opinion on this, and if other maintainers are ok
with it, then OK. But in the meantime, I think the previous version
was better.

> I have not made changes in places where more than 4-5 substitutions
> were necessary and where the code started looking unwieldy as a
> result.  I guess both those parameters are subjective, but I couldn't
> see a coding convention that seems to have been strictly followed
> throughout the code base.

I agree it's subjective. Just FYI, my tolerance starts at 2.  If I need
to repeat an expression, I often start thinking about factorizing into
constants, functions, etc (duplication is not the only part of the
decision process, so I don't necessarily do it).

-- 
Joel

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Pedro Alves]

On 09/26/2012 11:52 AM, Joel Brobecker wrote:
>>> Why is the type not OK for the assert, and yet OK for the rest of
>>> the code? (the same question applies to other files, as well)
>>
>> This is so that the assert is not subject to any truncation/overflow
>> resulting due to the type of LEN.  That way, I don't have to expand LEN
>> since I know that the value is always going to be less than 16 and if
>> something actually goes wrong, then the assert will definitely catch it.
> 
> OK. I see why it works.
> 
> But I can definitely see someone like myself missing that subtlety,
> and commit an obvious change that reduces the duplication by re-using
> the variable in the gdb_assert call. To the unattentive me, that's
> an obvious improvement. I think that's an issue.
> 
>> It does not make any functional difference at all and the justification
>> is in fact that it reduces the size of the bitpos patch.  I have
>> committed similar changes in the past that were deemed to be OK, so I
>> don't see why this patch in particular should be a problem.
> 
> I know it makes no functional difference, but it does make a noticeable
> difference in terms of maintenance, IMO. And I have in fact been
> silently grumbling about your patches being labeled "obvious" when
> in fact I do not consider them obvious. But I have let them go, because
> it wasn't significant enough that I felt I needed to talk about it.
> 
> But this part of this patch in particular did catch my attention, and
> I feel that we need to discuss it. I think this goes in the wrong
> direction, and it would be better to just change the variable
> (constant???) type, rather than duplicate the expression everywhere.
> I don't have a strong opinion on this, and if other maintainers are ok
> with it, then OK. But in the meantime, I think the previous version
> was better.

I agree with Joel, and was also silently glumbling.  :-)

> 
>> I have not made changes in places where more than 4-5 substitutions
>> were necessary and where the code started looking unwieldy as a
>> result.  I guess both those parameters are subjective, but I couldn't
>> see a coding convention that seems to have been strictly followed
>> throughout the code base.
> 
> I agree it's subjective. Just FYI, my tolerance starts at 2.  If I need
> to repeat an expression, I often start thinking about factorizing into
> constants, functions, etc (duplication is not the only part of the
> decision process, so I don't necessarily do it).

-- 
Pedro Alves

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Tom Tromey]

Joel> But I can definitely see someone like myself missing that subtlety,
Joel> and commit an obvious change that reduces the duplication by re-using
Joel> the variable in the gdb_assert call. To the unattentive me, that's
Joel> an obvious improvement. I think that's an issue.

Yeah, that's easy to picture happening.

It seems to me that a comment would be adequate.

Tom

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Siddhesh Poyarekar]

On Wed, 26 Sep 2012 15:58:55 +0100, Pedro wrote:
> 
> I agree with Joel, and was also silently glumbling.  :-)
> 

OK, should I go ahead and revert the patch as is or do you want me to
post it for review first?

Regards,
Siddhesh

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Joel Brobecker]

Hi Siddhesh,

> [1] http://sourceware.org/ml/gdb-cvs/2012-09/msg00147.html
> [2] http://sourceware.org/ml/gdb-patches/2012-08/msg00144.html
> 
>         * amd64-tdep.c (amd64_return_value): Use TYPE_LENGTH directly.
>         * bfin-tdep.c (bfin_extract_return_value): Likewise.
>         (bfin_store_return_value): Likewise.
>         * cris-tdep.c (cris_store_return_value): Likewise.
>         (cris_extract_return_value): Likewise.
>         * h8300-tdep.c (h8300_extract_return_value): Likewise.
>         * hppa-tdep.c (hppa64_return_value): Likewise.
>         * lm32-tdep.c (lm32_store_return_value): Likewise.
>         * microblaze-tdep.c (microblaze_store_return_value): Likewise.
>         * spu-tdep.c (spu_value_from_register): Likewise.
>         * vax-tdep.c (vax_return_value): Likewise.

Following the discussion that started at:
http://www.sourceware.org/ml/gdb-patches/2012-09/msg00570.html

Here is what I suggest we do:

> Index: gdb/amd64-tdep.c
[...]
>    gdb_assert (class[1] != AMD64_MEMORY);
> -  gdb_assert (len <= 16);
> +  gdb_assert (TYPE_LENGTH (type) <= 16);

Add a comment here to warn us against replacing the expression
with asserting on len, with an explanation as to why. This needs
to be done at every location where this trick is used.

Do you think it's better to do things this way rather than declare
len with the correct type?

> Index: gdb/cris-tdep.c
> Index: gdb/h8300-tdep.c
> Index: gdb/hppa-tdep.c
> Index: gdb/lm32-tdep.c
> Index: gdb/microblaze-tdep.c
> Index: gdb/spu-tdep.c
> Index: gdb/vax-tdep.c

Revert changes in those files, and declare the associated variable
with the correct type.

Thank you,
-- 
Joel

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Siddhesh Poyarekar]

On Thu, 27 Sep 2012 11:59:17 +0200, Joel wrote:
> > Index: gdb/amd64-tdep.c
> [...]
> >    gdb_assert (class[1] != AMD64_MEMORY);
> > -  gdb_assert (len <= 16);
> > +  gdb_assert (TYPE_LENGTH (type) <= 16);
> 
> Add a comment here to warn us against replacing the expression
> with asserting on len, with an explanation as to why. This needs
> to be done at every location where this trick is used.
> 
> Do you think it's better to do things this way rather than declare
> len with the correct type?

I'd rather just revert the whole patch. This is the only patch where I
have used this trick.  I'll post it for review in a while...

... or I can revert the commit as obvious if it's OK ;)

Regards,
Siddhesh

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Joel Brobecker]

> I'd rather just revert the whole patch. This is the only patch where I
> have used this trick.  I'll post it for review in a while...
> 
> ... or I can revert the commit as obvious if it's OK ;)

Or consider the revert pre-approved, your choice :).

-- 
Joel

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Siddhesh Poyarekar]

On Thu, 27 Sep 2012 12:19:19 +0200, Joel wrote:
> > I'd rather just revert the whole patch. This is the only patch
> > where I have used this trick.  I'll post it for review in a while...
> > 
> > ... or I can revert the commit as obvious if it's OK ;)
> 
> Or consider the revert pre-approved, your choice :).
> 

Done:

http://sourceware.org/ml/gdb-cvs/2012-09/msg00161.html

Regards,
Siddhesh

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Mark Kettenis]

> Date: Wed, 26 Sep 2012 12:52:06 +0200
> From: Joel Brobecker <brobecker@adacore.com>
> 
> > > Why is the type not OK for the assert, and yet OK for the rest of
> > > the code? (the same question applies to other files, as well)
> > 
> > This is so that the assert is not subject to any truncation/overflow
> > resulting due to the type of LEN.  That way, I don't have to expand LEN
> > since I know that the value is always going to be less than 16 and if
> > something actually goes wrong, then the assert will definitely catch it.
> 
> OK. I see why it works.
> 
> But I can definitely see someone like myself missing that subtlety,
> and commit an obvious change that reduces the duplication by re-using
> the variable in the gdb_assert call. To the unattentive me, that's
> an obvious improvement. I think that's an issue.
> 
> > It does not make any functional difference at all and the justification
> > is in fact that it reduces the size of the bitpos patch.  I have
> > committed similar changes in the past that were deemed to be OK, so I
> > don't see why this patch in particular should be a problem.
> 
> I know it makes no functional difference, but it does make a noticeable
> difference in terms of maintenance, IMO. And I have in fact been
> silently grumbling about your patches being labeled "obvious" when
> in fact I do not consider them obvious. But I have let them go, because
> it wasn't significant enough that I felt I needed to talk about it.
> 
> But this part of this patch in particular did catch my attention, and
> I feel that we need to discuss it. I think this goes in the wrong
> direction, and it would be better to just change the variable
> (constant???) type, rather than duplicate the expression everywhere.
> I don't have a strong opinion on this, and if other maintainers are ok
> with it, then OK. But in the meantime, I think the previous version
> was better.
> 
> > I have not made changes in places where more than 4-5 substitutions
> > were necessary and where the code started looking unwieldy as a
> > result.  I guess both those parameters are subjective, but I couldn't
> > see a coding convention that seems to have been strictly followed
> > throughout the code base.
> 
> I agree it's subjective. Just FYI, my tolerance starts at 2.  If I need
> to repeat an expression, I often start thinking about factorizing into
> constants, functions, etc (duplication is not the only part of the
> decision process, so I don't necessarily do it).

I agree with Joel.  Actually my tolerance starts at 1, if it avoids
having lines that are too long or if it reduces the number of nested
parentheses to a more manageable level.

Re: [commit][obv] Use TYPE_LENGTH directly where possible

[Jan Kratochvil]

On Fri, 28 Sep 2012 06:00:44 +0200, Mark Kettenis wrote:
> > Date: Wed, 26 Sep 2012 12:52:06 +0200
> > From: Joel Brobecker <brobecker@adacore.com>
> > I agree it's subjective. Just FYI, my tolerance starts at 2.  If I need
> > to repeat an expression, I often start thinking about factorizing into
> > constants, functions, etc (duplication is not the only part of the
> > decision process, so I don't necessarily do it).
> 
> I agree with Joel.  Actually my tolerance starts at 1, if it avoids
> having lines that are too long or if it reduces the number of nested
> parentheses to a more manageable level.

In my opinion it cannot be so generalized.  In a larger code using 'len'
variable may be for example less clear because its value may have become stale
after its original 'type' has been updated in the meantime etc.

Anyway thanks for the opinions, personally I do not have opinion on these
differences.

Although I found fine this kind of change (which I pre-approved to Siddhesh as
[obv] according to my opinion):
   ULONGEST val;
-  int len = TYPE_LENGTH (type);

-  if (len <= 4)
+  if (TYPE_LENGTH (type) <= 4)
     {
{ possibly more uses of 'len' }

But I no longer find fine this kind of change contained in this patch:
   int len = TYPE_LENGTH (type);
   ULONGEST c, addr;

-  switch (len)
+  switch (TYPE_LENGTH (type))
     {

because one should not check-in patch which brings the codebase to a state
which is not acceptable as the final solution, which this second case
acceptable is not.


Thanks,
Jan