[PATCH] dead code in mi-interp

[PATCH] dead code in mi-interp

[msnyder]

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

Must have been meant for something, but seems to have no side effects.


[-- Attachment #2: 287.txt --]
[-- Type: text/plain, Size: 1401 bytes --]

2007-08-08  Michael Snyder  <msnyder@access-company.com>

	* mi/mi-interp.c (mi_cmd_interpreter_exec): Dead code, dead variable.

Index: mi/mi-interp.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-interp.c,v
retrieving revision 1.20
diff -p -r1.20 mi-interp.c
*** mi/mi-interp.c	28 Apr 2007 21:52:38 -0000	1.20
--- mi/mi-interp.c	8 Aug 2007 22:23:16 -0000
*************** mi_cmd_interpreter_exec (char *command, 
*** 218,235 ****
  
    for (i = 1; i < argc; i++)
      {
-       char *buff = NULL;
-       /* Do this in a cleaner way...  We want to force execution to be
-          asynchronous for commands that run the target.  */
-       if (target_can_async_p () && (strcmp (argv[0], "console") == 0))
- 	{
- 	  int len = strlen (argv[i]);
- 	  buff = xmalloc (len + 2);
- 	  memcpy (buff, argv[i], len);
- 	  buff[len] = '&';
- 	  buff[len + 1] = '\0';
- 	}
- 
        /* We had to set sync_execution = 0 for the mi (well really for Project
           Builder's use of the mi - particularly so interrupting would work.
           But for console commands to work, we need to initialize it to 1 -
--- 218,223 ----
*************** mi_cmd_interpreter_exec (char *command, 
*** 245,251 ****
  	    break;
  	  }
        }
-       xfree (buff);
        do_exec_error_cleanups (ALL_CLEANUPS);
        sync_execution = 0;
      }
--- 233,238 ----

Re: [PATCH] dead code in mi-interp

[Nick Roberts]

 > Must have been meant for something, but seems to have no side effects.

There are probably no side effects because currently target_can_async_p ()
usually returns 0.  I hope to add an option to make GDB work asynchronously
after 6.7 is released.  Maybe this still won't be needed but I don't really
like these kinds of changes.  As they say: "If it ain't broke, don't fix it.".
There must be larger cobwebs to blow away.

 > 2007-08-08  Michael Snyder  <msnyder@access-company.com>
 > 
 > 	* mi/mi-interp.c (mi_cmd_interpreter_exec): Dead code, dead variable.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] dead code in mi-interp

[Jim Blandy]

Nick Roberts <nickrob@snap.net.nz> writes:
> There must be larger cobwebs to blow away.

But the larger cobwebs are not served to you on a silver Coverity
platter. :)

Re: [PATCH] dead code in mi-interp

[Daniel Jacobowitz]

On Thu, Aug 09, 2007 at 11:23:49AM +1200, Nick Roberts wrote:
>  > Must have been meant for something, but seems to have no side effects.
> 
> There are probably no side effects because currently target_can_async_p ()
> usually returns 0.  I hope to add an option to make GDB work asynchronously
> after 6.7 is released.  Maybe this still won't be needed but I don't really
> like these kinds of changes.  As they say: "If it ain't broke, don't fix it.".
> There must be larger cobwebs to blow away.

That's not what "no side effects" means - the code literally can't
ever have an effect.  It creates a string which nothing uses.  Why
keep it?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] dead code in mi-interp

[Nick Roberts]

 > That's not what "no side effects" means - the code literally can't
 > ever have an effect.  It creates a string which nothing uses.  Why
 > keep it?

OK, I hadn't realised that.  Unless the original author (Andrew Cagney?)
explains why it's not needed I would still prefer that it wasn't removed.  It
may be that it just wasn't hooked up because the asynchronous stuff was never
completed.  Once GDB can work asynchronously then it could be removed, if not
needed.  Presumably "no side effects" also means "can do no harm".

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] dead code in mi-interp

[msnyder]

>  > That's not what "no side effects" means - the code literally can't
>  > ever have an effect.  It creates a string which nothing uses.  Why
>  > keep it?
>
> OK, I hadn't realised that.  Unless the original author (Andrew Cagney?)
> explains why it's not needed I would still prefer that it wasn't removed.
> It
> may be that it just wasn't hooked up because the asynchronous stuff was
> never
> completed.  Once GDB can work asynchronously then it could be removed, if
> not
> needed.  Presumably "no side effects" also means "can do no harm".

Well, it can always be recovered from the CVS repository if it is
needed.  Personally I'd rathern not have dead code in there just
because it doesn't do any harm (unles it also has some benefit).

Re: [PATCH] dead code in mi-interp

[Bob Rossi]

On Thu, Aug 09, 2007 at 02:28:52PM -0700, msnyder@sonic.net wrote:
> >  > That's not what "no side effects" means - the code literally can't
> >  > ever have an effect.  It creates a string which nothing uses.  Why
> >  > keep it?
> >
> > OK, I hadn't realised that.  Unless the original author (Andrew Cagney?)
> > explains why it's not needed I would still prefer that it wasn't removed.
> > It
> > may be that it just wasn't hooked up because the asynchronous stuff was
> > never
> > completed.  Once GDB can work asynchronously then it could be removed, if
> > not
> > needed.  Presumably "no side effects" also means "can do no harm".
> 
> Well, it can always be recovered from the CVS repository if it is
> needed.  Personally I'd rathern not have dead code in there just
> because it doesn't do any harm (unles it also has some benefit).

Agreed.

Bob Rossi

Re: [PATCH] dead code in mi-interp

[Nick Roberts]

 > > It may be that it just wasn't hooked up because the asynchronous stuff was
 > > never completed.  Once GDB can work asynchronously then it could be
 > > removed, if not needed.  Presumably "no side effects" also means "can do
 > > no harm".
 > 
 > Well, it can always be recovered from the CVS repository if it is
 > needed.  Personally I'd rathern not have dead code in there just
 > because it doesn't do any harm (unles it also has some benefit).

You would only think of recovering it if you already knew it was there.  I've
just explained what I think is the benefit: they provide possible clues about
an asynchronous implementation.  This specific change is too small to worry
about but collectively I think you're erasing the past.  Maybe the code should
read:

        ...
	struct gdb_exception e = interp_exec (interp_to_use, buff);
                                                             ^^^^
Note that with synchronous execution you currently get:

(gdb) run &
Asynchronous execution not supported on this target.
(gdb) 

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [PATCH] dead code in mi-interp

[Bob Rossi]

On Fri, Aug 10, 2007 at 04:00:18PM +1200, Nick Roberts wrote:
>  > > It may be that it just wasn't hooked up because the asynchronous stuff was
>  > > never completed.  Once GDB can work asynchronously then it could be
>  > > removed, if not needed.  Presumably "no side effects" also means "can do
>  > > no harm".
>  > 
>  > Well, it can always be recovered from the CVS repository if it is
>  > needed.  Personally I'd rathern not have dead code in there just
>  > because it doesn't do any harm (unles it also has some benefit).
> 
> You would only think of recovering it if you already knew it was there.  I've
> just explained what I think is the benefit: they provide possible clues about
> an asynchronous implementation.

Even if that was true, the code should be commented out. It really is a
bad thing to have code in the program that is meaningless.

Bob Rossi

Re: [PATCH] dead code in mi-interp

[msnyder]

> On Fri, Aug 10, 2007 at 04:00:18PM +1200, Nick Roberts wrote:
>>  > > It may be that it just wasn't hooked up because the asynchronous
>> stuff was
>>  > > never completed.  Once GDB can work asynchronously then it could be
>>  > > removed, if not needed.  Presumably "no side effects" also means
>> "can do
>>  > > no harm".
>>  >
>>  > Well, it can always be recovered from the CVS repository if it is
>>  > needed.  Personally I'd rathern not have dead code in there just
>>  > because it doesn't do any harm (unles it also has some benefit).
>>
>> You would only think of recovering it if you already knew it was there.
>> I've
>> just explained what I think is the benefit: they provide possible clues
>> about
>> an asynchronous implementation.
>
> Even if that was true, the code should be commented out. It really is a
> bad thing to have code in the program that is meaningless.

And as I recall, the precident is that if you #if 0 something out,
you remove it.

ping Re: [PATCH] dead code in mi-interp

[msnyder]

>> On Fri, Aug 10, 2007 at 04:00:18PM +1200, Nick Roberts wrote:
>>>  > > It may be that it just wasn't hooked up because the asynchronous
>>> stuff was
>>>  > > never completed.  Once GDB can work asynchronously then it could
>>> be
>>>  > > removed, if not needed.  Presumably "no side effects" also means
>>> "can do
>>>  > > no harm".
>>>  >
>>>  > Well, it can always be recovered from the CVS repository if it is
>>>  > needed.  Personally I'd rathern not have dead code in there just
>>>  > because it doesn't do any harm (unles it also has some benefit).
>>>
>>> You would only think of recovering it if you already knew it was there.
>>> I've
>>> just explained what I think is the benefit: they provide possible clues
>>> about
>>> an asynchronous implementation.
>>
>> Even if that was true, the code should be commented out. It really is a
>> bad thing to have code in the program that is meaningless.
>
> And as I recall, the precident is that if you #if 0 something out,
> you remove it.

So it seems like three people favor removing it, and one person opposes.

Can we reach a resolution?

Re: ping Re: [PATCH] dead code in mi-interp

[Nick Roberts]

 > So it seems like three people favor removing it, and one person opposes.
 > 
 > Can we reach a resolution?

I don't feel that strongly about it and, in any case, the MAINTAINERS file says
that you have the authority to make the change.  Go for it.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: ping Re: [PATCH] dead code in mi-interp

[msnyder]

>  > So it seems like three people favor removing it, and one person
> opposes.
>  >
>  > Can we reach a resolution?
>
> I don't feel that strongly about it and, in any case, the MAINTAINERS file
> says
> that you have the authority to make the change.  Go for it.

Thanks, Nick.
Committing.