[RFA] gdbserver/server.c: Replace 2x strlen() by a variable

[RFA] gdbserver/server.c: Replace 2x strlen() by a variable

[Markus Deuling]

Hello,

this little patch adds a variable to prevent strlen() from being called two times.
Maybe it would also be a good idea to replace it by the hard-coded length, but I think
the code then will be less readable then.

Is it ok to apply ?

ChangeLog:

	* server.c (handle_general_set): New variable len instead
	of using strlen two times.

=========================================
diff -urN src/gdb/gdbserver/server.c dev/gdb/gdbserver/server.c
--- src/gdb/gdbserver/server.c	2006-12-21 08:38:11.000000000 +0100
+++ dev/gdb/gdbserver/server.c	2006-12-21 08:38:25.000000000 +0100
@@ -163,10 +163,11 @@
 void
 handle_general_set (char *own_buf)
 {
-  if (strncmp ("QPassSignals:", own_buf, strlen ("QPassSignals:")) == 0)
+  int len = strlen ("QPassSignals:");
+  if (strncmp ("QPassSignals:", own_buf, len) == 0)
     {
       int numsigs = (int) TARGET_SIGNAL_LAST, i;
-      const char *p = own_buf + strlen ("QPassSignals:");
+      const char *p = own_buf + len;
       CORE_ADDR cursig;
 
       p = decode_address_to_semicolon (&cursig, p);

Regards,
Markus

-- 
Markus Deuling
GNU Toolchain for Linux on Cell BE
deuling@de.ibm.com 

Re: [RFA] gdbserver/server.c: Replace 2x strlen() by a variable

[Joel Brobecker]

> ChangeLog:
> 
> 	* server.c (handle_general_set): New variable len instead
> 	of using strlen two times.

Actually, since the same string is duplicated a couple of times,
I would also suggest declaring a constant string "QPassSignals:"
and use the constant instead of risking a typo... How about:

   const char str[] = "QPassSignals:";

and then use "sizeof (str) - 1". Is that bad coding style? Otherwise,
you can declare your sale constant:

   const int len = strlen (str);

I am not reviewer, so these are just suggestions, not a request (JIC).

> =========================================
> diff -urN src/gdb/gdbserver/server.c dev/gdb/gdbserver/server.c
> --- src/gdb/gdbserver/server.c	2006-12-21 08:38:11.000000000 +0100
> +++ dev/gdb/gdbserver/server.c	2006-12-21 08:38:25.000000000 +0100
> @@ -163,10 +163,11 @@
> void
> handle_general_set (char *own_buf)
> {
> -  if (strncmp ("QPassSignals:", own_buf, strlen ("QPassSignals:")) == 0)
> +  int len = strlen ("QPassSignals:");
> +  if (strncmp ("QPassSignals:", own_buf, len) == 0)
>     {
>       int numsigs = (int) TARGET_SIGNAL_LAST, i;
> -      const char *p = own_buf + strlen ("QPassSignals:");
> +      const char *p = own_buf + len;
>       CORE_ADDR cursig;
> 
>       p = decode_address_to_semicolon (&cursig, p);
> 
> Regards,
> Markus
> 
> -- 
> Markus Deuling
> GNU Toolchain for Linux on Cell BE
> deuling@de.ibm.com 

-- 
Joel

Re: [RFA] gdbserver/server.c: Replace 2x strlen() by a variable

[Andreas Schwab]

Joel Brobecker <brobecker@adacore.com> writes:

>> ChangeLog:
>> 
>> 	* server.c (handle_general_set): New variable len instead
>> 	of using strlen two times.
>
> Actually, since the same string is duplicated a couple of times,
> I would also suggest declaring a constant string "QPassSignals:"
> and use the constant instead of risking a typo... How about:
>
>    const char str[] = "QPassSignals:";

Or just define it as a macro.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] gdbserver/server.c: Replace 2x strlen() by a variable

[Mark Kettenis]

> > ChangeLog:
> >
> > 	* server.c (handle_general_set): New variable len instead
> > 	of using strlen two times.
>
>  Actually, since the same string is duplicated a couple of times,
>  I would also suggest declaring a constant string "QPassSignals:"
>  and use the constant instead of risking a typo... How about:
>
>     const char str[] = "QPassSignals:";
>
>  and then use "sizeof (str) - 1". Is that bad coding style? Otherwise,
>  you can declare your sale constant:
>
>     const int len = strlen (str);
>
>  I am not reviewer, so these are just suggestions, not a request (JIC).

I'm pretty sure GCC will optimize away the strlen("QPassSignals:") anyway,
so we really shouldn't try to obfuscate the code just to make it a bit
faster.  So if optimization was the Markus' argument for making this
change I object to this change.

Mark

Re: [RFA] gdbserver/server.c: Replace 2x strlen() by a variable

[Joel Brobecker]

> >  Actually, since the same string is duplicated a couple of times,
> >  I would also suggest declaring a constant string "QPassSignals:"
> >  and use the constant instead of risking a typo... How about:
> >
> >     const char str[] = "QPassSignals:";
> >
> >  and then use "sizeof (str) - 1". Is that bad coding style? Otherwise,
> >  you can declare your sale constant:
> >
> >     const int len = strlen (str);
> >
> >  I am not reviewer, so these are just suggestions, not a request (JIC).
> 
> I'm pretty sure GCC will optimize away the strlen("QPassSignals:") anyway,
> so we really shouldn't try to obfuscate the code just to make it a bit
> faster.  So if optimization was the Markus' argument for making this
> change I object to this change.

How about avoiding the string duplication? Do you object to that too?
This change seems like a good idea to me - although I agree that the
risks of inconsistency are very small...

-- 
Joel

Re: [RFA] gdbserver/server.c: Replace 2x strlen() by a variable

[Daniel Jacobowitz]

On Thu, Dec 21, 2006 at 05:05:17PM +0400, Joel Brobecker wrote:
> > I'm pretty sure GCC will optimize away the strlen("QPassSignals:") anyway,
> > so we really shouldn't try to obfuscate the code just to make it a bit
> > faster.  So if optimization was the Markus' argument for making this
> > change I object to this change.

Mark is correct, at least for recent versions of GCC.  strlen of a
constant string without embedded zeros is folded to sizeof the string
minus one.

> How about avoiding the string duplication? Do you object to that too?
> This change seems like a good idea to me - although I agree that the
> risks of inconsistency are very small...

If it makes the code harder to read, I do object.  This, and similar
motifs with hardcoded lengths instead of duplicated strlens, are all
through that file.  I was careful to keep the duplications close
together so that they are easily visually confirmed.  I don't want to
have to hunt around for the code implementing a particular packet.

If it makes the code easier to read somehow, I don't object - I'd
have to see it.

-- 
Daniel Jacobowitz
CodeSourcery