Delete struct inferior_suspend_state

  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.
  

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
  

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.
  

> 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.
  

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
  

> 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.
  

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. :-)
  

> 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.
  

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 :-)).
  

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
  

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.