[ob] remote.c, eliminate unused variables

[ob] remote.c, eliminate unused variables

[Michael Snyder]

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



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

2010-05-05  Michael Snyder  <msnyder@vmware.com>

	* remote.c (remote_threads_info): Delete unused variable.
	(process_stop_reply): Delete unused variable.
	(remote_get_trace_status): Delete unused variables.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.405
diff -u -p -r1.405 remote.c
--- remote.c	5 May 2010 15:05:57 -0000	1.405
+++ remote.c	5 May 2010 20:43:46 -0000
@@ -2512,8 +2512,8 @@ remote_threads_info (struct target_ops *
 	{
 	  struct gdb_xml_parser *parser;
 	  struct threads_parsing_context context;
-	  struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
 
+	  make_cleanup (null_cleanup, NULL);
 	  context.items = 0;
 	  parser = gdb_xml_create_parser_and_cleanup (_("threads"),
 						      threads_elements,
@@ -5079,7 +5079,6 @@ process_stop_reply (struct stop_reply *s
 		    struct target_waitstatus *status)
 {
   ptid_t ptid;
-  struct thread_info *info;
 
   *status = stop_reply->ws;
   ptid = stop_reply->ptid;
@@ -9637,8 +9636,7 @@ remote_trace_start (void)
 static int
 remote_get_trace_status (struct trace_status *ts)
 {
-  char *p, *p1, *p_temp;
-  ULONGEST val;
+  char *p;
   /* FIXME we need to get register block size some other way */
   extern int trace_regblock_size;
   trace_regblock_size = get_remote_arch_state ()->sizeof_g_packet;

Re: [ob] remote.c, eliminate unused variables

[Pedro Alves]

On Wednesday 05 May 2010 21:46:55, Michael Snyder wrote:
>   2010-05-05  Michael Snyder  <msnyder@vmware.com>
> 
>         * remote.c (remote_threads_info): Delete unused variable.
>         (process_stop_reply): Delete unused variable.
>         (remote_get_trace_status): Delete unused variables.
> 
> Index: remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.405
> diff -u -p -r1.405 remote.c
> --- remote.c    5 May 2010 15:05:57 -0000       1.405
> +++ remote.c    5 May 2010 20:43:46 -0000
> @@ -2512,8 +2512,8 @@ remote_threads_info (struct target_ops *
>         {
>           struct gdb_xml_parser *parser;
>           struct threads_parsing_context context;
> -         struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
>  
> +         make_cleanup (null_cleanup, NULL);

Are you making sure (in all your patches) that the reason the
variables are unused isn't itself a bug?  In this case, creating a
null_cleanup and not storing a pointer anywhere is
highly suspicious...

-- 
Pedro Alves

Re: [ob] remote.c, eliminate unused variables

[Michael Snyder]

Pedro Alves wrote:
> On Wednesday 05 May 2010 21:46:55, Michael Snyder wrote:
>>   2010-05-05  Michael Snyder  <msnyder@vmware.com>
>>
>>         * remote.c (remote_threads_info): Delete unused variable.
>>         (process_stop_reply): Delete unused variable.
>>         (remote_get_trace_status): Delete unused variables.
>>
>> Index: remote.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/remote.c,v
>> retrieving revision 1.405
>> diff -u -p -r1.405 remote.c
>> --- remote.c    5 May 2010 15:05:57 -0000       1.405
>> +++ remote.c    5 May 2010 20:43:46 -0000
>> @@ -2512,8 +2512,8 @@ remote_threads_info (struct target_ops *
>>         {
>>           struct gdb_xml_parser *parser;
>>           struct threads_parsing_context context;
>> -         struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
>>  
>> +         make_cleanup (null_cleanup, NULL);
> 
> Are you making sure (in all your patches) that the reason the
> variables are unused isn't itself a bug? 

Can't guarantee it, no.
I'm making sure the semantics isn't changed, but I can't always
be sure that the original semantics was right.

 > In this case, creating a
> null_cleanup and not storing a pointer anywhere is
> highly suspicious...

Well, then it was wrong when I got there.  The variable that
it was stored to was not used, but shadowed an outer scope variable
of the same name, which was used.

Maybe it should have stored it without declaring it?
I  don't know...  what do you think?

Re: [ob] remote.c, eliminate unused variables

[Pedro Alves]

On Wednesday 05 May 2010 22:14:17, Michael Snyder wrote:
> Pedro Alves wrote:
> > On Wednesday 05 May 2010 21:46:55, Michael Snyder wrote:

> >> --- remote.c    5 May 2010 15:05:57 -0000       1.405
> >> +++ remote.c    5 May 2010 20:43:46 -0000
> >> @@ -2512,8 +2512,8 @@ remote_threads_info (struct target_ops *
> >>         {
> >>           struct gdb_xml_parser *parser;
> >>           struct threads_parsing_context context;
> >> -         struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
> >>  
> >> +         make_cleanup (null_cleanup, NULL);
> > 
> > Are you making sure (in all your patches) that the reason the
> > variables are unused isn't itself a bug? 
> 
> Can't guarantee it, no.
> I'm making sure the semantics isn't changed, but I can't always
> be sure that the original semantics was right.

Well, then I'll ask please, don't "fix" more things like this,
and surely don't call it obvious.  You're removing a warning for
the sake of it.  A warning is useful as a hint at something 
wrong with the code; there may be something genuinely wrong
with it.  Removing it blindly removes the useful hint.  If you
want to be bothered to look at the code to see if there's
something else genuinely wrong, then please, don't change it.

>  > In this case, creating a
> > null_cleanup and not storing a pointer anywhere is
> > highly suspicious...
> 
> Well, then it was wrong when I got there.  The variable that
> it was stored to was not used, but shadowed an outer scope variable
> of the same name, which was used.
> 
> Maybe it should have stored it without declaring it?

Store what?  What do you mean?

> I  don't know...  what do you think?

null_cleanup's only serve one single purpose --- to
introduce a new cleanup scope, so you can do things like:

   struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
   ...
   foo = xmalloc (big_chunk);
   make_cleanup (xfree, foo);
   ...
   bar = xmalloc (big_chunk2);
   make_cleanup (foo, bar);
   ...
   baz = xmalloc (big_chunk3);
   make_cleanup (bar, baz);
   ...
   do_cleanup (old_chain);

Note no pointer is stored for the intervening cleanups.

In this particular case, in remote.c, there's no other cleanup
created in the scope in question, and the outer scope already
has a proper do_cleanups, so the right thing to do is to
remove the whole `make_cleanups (null_cleanup, NULL)' line.

But, and here's the genuine bug, if gdb_xml_parse throws
midway while parsing the xml, there's no cleanup set to
release `context.items'.  There's an usual pattern around
all these `gdb_xml_create_parser_and_cleanup' calls that
this particular case isn't following (and the cleanup in
question may indicate that it was simply forgotten, and
so the warning was being useful).

-- 
Pedro Alves

Re: [ob] remote.c, eliminate unused variables

[Michael Snyder]

Pedro Alves wrote:
> On Wednesday 05 May 2010 22:14:17, Michael Snyder wrote:
>> Pedro Alves wrote:
>>> On Wednesday 05 May 2010 21:46:55, Michael Snyder wrote:
> 
>>>> --- remote.c    5 May 2010 15:05:57 -0000       1.405
>>>> +++ remote.c    5 May 2010 20:43:46 -0000
>>>> @@ -2512,8 +2512,8 @@ remote_threads_info (struct target_ops *
>>>>         {
>>>>           struct gdb_xml_parser *parser;
>>>>           struct threads_parsing_context context;
>>>> -         struct cleanup *back_to = make_cleanup (null_cleanup, NULL);
>>>>  
>>>> +         make_cleanup (null_cleanup, NULL);
>>> Are you making sure (in all your patches) that the reason the
>>> variables are unused isn't itself a bug? 
>> Can't guarantee it, no.
>> I'm making sure the semantics isn't changed, but I can't always
>> be sure that the original semantics was right.
> 
> Well, then I'll ask please, don't "fix" more things like this,
> and surely don't call it obvious.  You're removing a warning for
> the sake of it. 

No, I'm attempting to make the code easier to understand by
removing dead code and variables.  Since this warning is
turned off, I'm not even reducing the number of warnings.

> A warning is useful as a hint at something 
> wrong with the code; there may be something genuinely wrong
> with it.  Removing it blindly removes the useful hint. 

There's no hint if the warning is turned off.  If I hadn't
touched it and you hadn't reviewed my change, it would have
remained undiscovered indefinitely.

So let's fix it, shall we?  I'll post a separate patch for you to review.


> If you
> want to be bothered to look at the code to see if there's
> something else genuinely wrong, then please, don't change it.

That's not fair, I did "bother" to look at the code.
One got by me, that's all.  Thanks for catching it.

Re: [ob] remote.c, eliminate unused variables

[Pedro Alves]

On Wednesday 05 May 2010 23:22:20, Michael Snyder wrote:
> >> Can't guarantee it, no.
> >> I'm making sure the semantics isn't changed, but I can't always
> >> be sure that the original semantics was right.
> > 
> > Well, then I'll ask please, don't "fix" more things like this,
> > and surely don't call it obvious.  You're removing a warning for
> > the sake of it. 
> 
> No, I'm attempting to make the code easier to understand by
> removing dead code and variables.  Since this warning is
> turned off, I'm not even reducing the number of warnings.

You're hiding the bug for whoever wants to catch these
bugs by enabling the warning in its local tree.  That's what
I mean: you must be enabling the warning explicitly to see
these; if one doesn't want to check if the warnings are
pointing at something wrong, then one just shouldn't enable
the warning in the first place.

> > A warning is useful as a hint at something 
> > wrong with the code; there may be something genuinely wrong
> > with it.  Removing it blindly removes the useful hint. 
> 
> There's no hint if the warning is turned off.  If I hadn't
> touched it and you hadn't reviewed my change, it would have
> remained undiscovered indefinitely.

I'll see it the same way you must be seeing it.  By
enabling the warning on my local build.

> So let's fix it, shall we?  I'll post a separate patch for you to review.

To be clear, I'm very much not interested in reviewing these
kind of patches.  That would mean doing about the same work
the person writing the patch is already doing.  What I'm intersted
in, is making sure that whenever these patches go in, it was made sure
the variables weren't being unused because we forgot to use
them, instead of just deleting them.  If there's any doubt
that's the case, then the patch isn't obvious, and it should
be posted for comments.

> > If you
> > want to be bothered to look at the code to see if there's
> > something else genuinely wrong, then please, don't change it.
> 
> That's not fair, I did "bother" to look at the code.
> One got by me, that's all.  Thanks for catching it.

Okay, thanks.  I misunderstood perhaps, your reply seemed
to imply you didn't ("Are you making sure" -> "Can't guarantee it, no").

-- 
Pedro Alves

Re: [ob] remote.c, eliminate unused variables

[Tom Tromey]

>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:

Michael> No, I'm attempting to make the code easier to understand by
Michael> removing dead code and variables.  Since this warning is
Michael> turned off, I'm not even reducing the number of warnings.

Are you planning to enable them?  I hope so, since otherwise this work
will quickly regress.

FWIW I read through all the patches and sent comments about the changes
I saw that seemed fishy.

Tom

Re: [ob] remote.c, eliminate unused variables

[Michael Snyder]

Tom Tromey wrote:
>>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:
> 
> Michael> No, I'm attempting to make the code easier to understand by
> Michael> removing dead code and variables.  Since this warning is
> Michael> turned off, I'm not even reducing the number of warnings.
> 
> Are you planning to enable them?  I hope so, since otherwise this work
> will quickly regress.

I was, but there have been a handful of cases that I could not eliminate 
so far.

> 
> FWIW I read through all the patches and sent comments about the changes
> I saw that seemed fishy.

Thanks, the review is much appreciated.