[PATCH] Delete struct inferior_suspend_state

[PATCH] Delete struct inferior_suspend_state

[Doug Evans]

Hi.

I happened across some #if 0's in the code and thought that odd.

I found the relevant thread here:

https://sourceware.org/ml/gdb-patches/2012-06/msg00370.html

Any desire to continue to keep this, or can we delete it?
[I don't have a strong preference, but it feels like it's time.]

2014-07-31  Doug Evans  <dje@google.com>

	* inferior.h (struct inferior_suspend_state): Delete, unused.
	All references deleted.

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 522b674..623977b 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -129,9 +129,7 @@ struct thread_control_state
   struct interp *command_interp;
 };
 
-/* Inferior thread specific part of `struct infcall_suspend_state'.
-
-   Inferior process counterpart is `struct inferior_suspend_state'.  */
+/* Inferior thread specific part of `struct infcall_suspend_state'.  */
 
 struct thread_suspend_state
 {
diff --git a/gdb/inferior.h b/gdb/inferior.h
index 668c888..3929d16 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -272,16 +272,6 @@ struct inferior_control_state
   enum stop_kind stop_soon;
 };
 
-/* Inferior process specific part of `struct infcall_suspend_state'.
-
-   Inferior thread counterpart is `struct thread_suspend_state'.  */
-
-#if 0 /* Currently unused and empty structures are not valid C.  */
-struct inferior_suspend_state
-{
-};
-#endif
-
 /* GDB represents the state of each program execution with an object
    called an inferior.  An inferior typically corresponds to a process
    but is more general and applies also to targets that do not have a
@@ -310,12 +300,6 @@ struct inferior
      See `struct inferior_control_state'.  */
   struct inferior_control_state control;
 
-  /* State of inferior process to restore after GDB is done with an inferior
-     call.  See `struct inferior_suspend_state'.  */
-#if 0 /* Currently unused and empty structures are not valid C.  */
-  struct inferior_suspend_state suspend;
-#endif
-
   /* True if this was an auto-created inferior, e.g. created from
      following a fork; false, if this inferior was manually added by
      the user, and we should not attempt to prune it
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 33aa674..08b5556 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6819,9 +6819,6 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var,
 struct infcall_suspend_state
 {
   struct thread_suspend_state thread_suspend;
-#if 0 /* Currently unused and empty structures are not valid C.  */
-  struct inferior_suspend_state inferior_suspend;
-#endif
 
   /* Other fields:  */
   CORE_ADDR stop_pc;
@@ -6841,9 +6838,6 @@ save_infcall_suspend_state (void)
 {
   struct infcall_suspend_state *inf_state;
   struct thread_info *tp = inferior_thread ();
-#if 0
-  struct inferior *inf = current_inferior ();
-#endif
   struct regcache *regcache = get_current_regcache ();
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   gdb_byte *siginfo_data = NULL;
@@ -6877,9 +6871,6 @@ save_infcall_suspend_state (void)
     }
 
   inf_state->thread_suspend = tp->suspend;
-#if 0 /* Currently unused and empty structures are not valid C.  */
-  inf_state->inferior_suspend = inf->suspend;
-#endif
 
   /* run_inferior_call will not use the signal due to its `proceed' call with
      GDB_SIGNAL_0 anyway.  */
@@ -6898,16 +6889,10 @@ void
 restore_infcall_suspend_state (struct infcall_suspend_state *inf_state)
 {
   struct thread_info *tp = inferior_thread ();
-#if 0
-  struct inferior *inf = current_inferior ();
-#endif
   struct regcache *regcache = get_current_regcache ();
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
 
   tp->suspend = inf_state->thread_suspend;
-#if 0 /* Currently unused and empty structures are not valid C.  */
-  inf->suspend = inf_state->inferior_suspend;
-#endif
 
   stop_pc = inf_state->stop_pc;
 

Re: [PATCH] Delete struct inferior_suspend_state

[Jan Kratochvil]

Hi,

I do not mind any way.

But in the case it is deleted I believe it is better to keep there at least
comments at the places where is currently #if 0.  So if one is adding a new
field one immediately knows where it should belong - to the 4 cases of 2x2
matrix {thread,inferior}_{suspend,control}_state - in the case it would belong
to the inferior+suspend combination.


Jan

Re: [PATCH] Delete struct inferior_suspend_state

[Doug Evans]

On Thu, Jul 31, 2014 at 12:30 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Hi,
>
> I do not mind any way.
>
> But in the case it is deleted I believe it is better to keep there at least
> comments at the places where is currently #if 0.  So if one is adding a new
> field one immediately knows where it should belong - to the 4 cases of 2x2
> matrix {thread,inferior}_{suspend,control}_state - in the case it would belong
> to the inferior+suspend combination.

Thanks.

If we're going to keep comments we might as well keep the #if 0'd code
(which is fine by me).
There's no real difference between the two, and the #if 0'd out code
is more descriptive, though I would add a commit (tweak a comment?) so
that the next person will more easily know that the #if 0's are ok.
IWBN to have examples where #if 0 is at least not a bad thing.

Re: [PATCH] Delete struct inferior_suspend_state

[Joel Brobecker]

> If we're going to keep comments we might as well keep the #if 0'd code
> (which is fine by me).
> There's no real difference between the two, and the #if 0'd out code
> is more descriptive, though I would add a commit (tweak a comment?) so
> that the next person will more easily know that the #if 0's are ok.
> IWBN to have examples where #if 0 is at least not a bad thing.

In my experience, #if 0'ed code has been bit-rotting. Better, IMO,
to just be as descriptive as possible in the code, even if that
involves naming functions, etc. That way, we can maintain the
description a little better.

-- 
Joel

Re: [PATCH] Delete struct inferior_suspend_state

[Doug Evans]

On Thu, Jul 31, 2014 at 1:05 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> If we're going to keep comments we might as well keep the #if 0'd code
>> (which is fine by me).
>> There's no real difference between the two, and the #if 0'd out code
>> is more descriptive, though I would add a commit (tweak a comment?) so
>> that the next person will more easily know that the #if 0's are ok.
>> IWBN to have examples where #if 0 is at least not a bad thing.
>
> In my experience, #if 0'ed code has been bit-rotting. Better, IMO,
> to just be as descriptive as possible in the code, even if that
> involves naming functions, etc. That way, we can maintain the
> description a little better.

Comments don't necessarily fare much better.
Consider the 23 year old TODO. 1/2 :-)
ref: https://sourceware.org/ml/gdb-patches/2014-07/msg00832.html

Re: [PATCH] Delete struct inferior_suspend_state

[Joel Brobecker]

> Comments don't necessarily fare much better.
> Consider the 23 year old TODO. 1/2 :-)
> ref: https://sourceware.org/ml/gdb-patches/2014-07/msg00832.html

That is sometimes true, but not always. Comments are easier to
search/replace, while commented out code is harder to maintain. Consider
for instance the case of modifying a function where you want to add
a parameter. With code, you'd have to fix it the best you can, while
you wouldn't with a simple reference to that function in the comment.

-- 
Joel

Re: [PATCH] Delete struct inferior_suspend_state

[Doug Evans]

On Thu, Jul 31, 2014 at 1:42 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> Comments don't necessarily fare much better.
>> Consider the 23 year old TODO. 1/2 :-)
>> ref: https://sourceware.org/ml/gdb-patches/2014-07/msg00832.html
>
> That is sometimes true, but not always. Comments are easier to
> search/replace, while commented out code is harder to maintain. Consider
> for instance the case of modifying a function where you want to add
> a parameter. With code, you'd have to fix it the best you can, while
> you wouldn't with a simple reference to that function in the comment.

That's a general concern, yes.
My comments in this thread are confined to the particular case at hand.

It would be interesting to do an audit and see how many outdated
comments gdb has.
Plus it's not clear to me comments are easier to search - what search
key do I use if I'm not grepping for a function name, etc.?

While I can envision code changes that obsolete what's currently #if
0'd out (someone renames "inf", "inf_state", or some such), for the
particular case at hand, I don't mind what's there ---  the patch
that's there got checked in, so I'm assuming I'm not the only one. :-)

Re: [PATCH] Delete struct inferior_suspend_state

[Joel Brobecker]

> That's a general concern, yes.
> My comments in this thread are confined to the particular case at hand.

Jan was saying that he does not mind it if the code gets removed.
So I think we can remove it.

Jan also asked that we add a comment to mark the location where
the code should be add should we need it. I don't mind, but
these are about something that has become a fairly general and
standard paradigm, so I don't personally really see much usefulness
in that, while things may change in the future, making those
comments incomplete or incorrect.

Commented-out code should be avoided, and removed if found.
There are no absolute rules as far as I am concerned, so we may
one day face a situation where it's best to ignore that rule.
But we'd have to justify why that's best before going ahead.

In this particular case IMO, the convenience doesn't justify us
keeping the code around, so I would go ahead removing it.

-- 
Joel

Re: [PATCH] Delete struct inferior_suspend_state

[Doug Evans]

On Thu, Jul 31, 2014 at 1:58 PM, Doug Evans <dje@google.com> wrote:
> [...]
> It would be interesting to do an audit and see how many outdated
> comments gdb has.
> [...]

I'm happy to go with whatever the community wants (I want to be clear
that I'm not trying to advance any particular position on this patch -
I do have opinions on how to handle the various choices, but I don't
favor any particular choice).

But as a data point to the above comment, because I think it's an
important issue,
this comment in inf-loop.c is odd.  I happened across it because I'm
trying to implement having the event loop handle waiting for all
threads to stop, instead of doing the waiting in a special loop apart
from the event loop.

inf-loop.c:

/* General function to handle events in the inferior.  So far it just
   takes care of detecting errors reported by select() or poll(),
   otherwise it assumes that all is OK, and goes on reading data from
   the fd.  This however may not always be what we want to do.  */
void
inferior_event_handler (enum inferior_event_type event_type,
                        gdb_client_data client_data)
{
  ...

AFAICT, it doesn't match what the function does at all.

inferior_event_handler + fetch_inferior_event is also a bit odd.
I see some cleanup potential here, more on that later (I hope, pending
finding the time :-)).

Re: [PATCH] Delete struct inferior_suspend_state

[Stan Shebs]

On 7/31/14, 12:10 PM, Doug Evans wrote:
> Hi.
> 
> I happened across some #if 0's in the code and thought that odd.
> 
> I found the relevant thread here:
> 
> https://sourceware.org/ml/gdb-patches/2012-06/msg00370.html
> 
> Any desire to continue to keep this, or can we delete it?
> [I don't have a strong preference, but it feels like it's time.]

I was sure we had set a policy to delete code instead of doing #if 0,
but I can't find anything in writing that says so.

In any case, retaining dead code seems pointless when version control
systems make it easy to find again.

Stan
stan@codesourcery.com

Re: [PATCH] Delete struct inferior_suspend_state

[Doug Evans]

On Fri, Aug 8, 2014 at 4:25 PM, Stan Shebs <stanshebs@earthlink.net> wrote:
> On 7/31/14, 12:10 PM, Doug Evans wrote:
>> Hi.
>>
>> I happened across some #if 0's in the code and thought that odd.
>>
>> I found the relevant thread here:
>>
>> https://sourceware.org/ml/gdb-patches/2012-06/msg00370.html
>>
>> Any desire to continue to keep this, or can we delete it?
>> [I don't have a strong preference, but it feels like it's time.]
>
> I was sure we had set a policy to delete code instead of doing #if 0,
> but I can't find anything in writing that says so.
>
> In any case, retaining dead code seems pointless when version control
> systems make it easy to find again.

... assuming you know it's there to look for.
In this particular case, the intent is to leave documentation on how
it's intended a particular bit of code be extended (in part because C
doesn't allow empty structs so a compromise is made).
Would anyone faced with a particular situation such as this go back
through the repo looking just in case such documentation was there and
then got deleted?  Perhaps, but I'd totally understand if someone
ended up putting time into it only to be told a decision has been made
to do it differently and being disappointed that the documentation of
that decision was in deleted text recorded only in the repo.  [It may
turn out that the "decision" is not an absolute one of course, but
it's still documentation we're talking about, not (dead) code.]

Maybe in addition to a rule to not use #if 0, we also need a rule to
not document how it's intended code be extended (at least not as the
code).
I can see a case made for either side of this, depending on the situation.