[2/3] Don't set terminal flags twice in a row

Message ID 1509635522-16945-3-git-send-email-palves@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves Nov. 2, 2017, 3:12 p.m. UTC
  I find this odd 'set flags twice' ancient code and comment annoyingly
distracting.  It may well be that the reason for the double-set was
simply a copy/paste mistake, and that we've been doing this for
decades [1] for no good reason.  Let's just get rid of it, and if we
find a real reason, add it back with a comment explaining why it's
necessary.

[1] This double-set was already in gdb 2.4 / 1988, the oldest release
we have sources for, and imported in git.  From 'git show 7b4ac7e1ed2c
inflow.c':

   +void
   +terminal_inferior ()
   +{
   +  if (terminal_is_ours)   /*  && inferior_thisrun_terminal == 0) */
   +    {
   +      fcntl (0, F_SETFL, tflags_inferior);
   +      fcntl (0, F_SETFL, tflags_inferior);

The "is there a reason" comment was added in 1993, by:

  commit a88797b5eadf31e21804bc820429028bf708fbcd
  Author:     Fred Fish <fnf@specifix.com>
  AuthorDate: Thu Aug 5 01:33:45 1993 +0000

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* inflow.c (child_terminal_inferior, child_terminal_ours_1): No
	longer set flags twice in row.
---
 gdb/inflow.c | 9 ---------
 1 file changed, 9 deletions(-)
  

Comments

Sergio Durigan Junior Nov. 2, 2017, 6:34 p.m. UTC | #1
On Thursday, November 02 2017, Pedro Alves wrote:

> I find this odd 'set flags twice' ancient code and comment annoyingly
> distracting.  It may well be that the reason for the double-set was
> simply a copy/paste mistake, and that we've been doing this for
> decades [1] for no good reason.  Let's just get rid of it, and if we
> find a real reason, add it back with a comment explaining why it's
> necessary.
>
> [1] This double-set was already in gdb 2.4 / 1988, the oldest release
> we have sources for, and imported in git.  From 'git show 7b4ac7e1ed2c
> inflow.c':
>
>    +void
>    +terminal_inferior ()
>    +{
>    +  if (terminal_is_ours)   /*  && inferior_thisrun_terminal == 0) */
>    +    {
>    +      fcntl (0, F_SETFL, tflags_inferior);
>    +      fcntl (0, F_SETFL, tflags_inferior);
>
> The "is there a reason" comment was added in 1993, by:
>
>   commit a88797b5eadf31e21804bc820429028bf708fbcd
>   Author:     Fred Fish <fnf@specifix.com>
>   AuthorDate: Thu Aug 5 01:33:45 1993 +0000

FWIW, I've stumbled upon this part while doing the startup-with-shell
work, and it also intrigued me.  Anyway, I agree that we should remove
this double-set.  Thanks for doing that.

>
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>
> 	* inflow.c (child_terminal_inferior, child_terminal_ours_1): No
> 	longer set flags twice in row.
> ---
>  gdb/inflow.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index a96d4fc..d46d693 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -244,10 +244,6 @@ child_terminal_inferior (struct target_ops *self)
>        int result;
>  
>  #ifdef F_GETFL
> -      /* Is there a reason this is being done twice?  It happens both
> -         places we use F_SETFL, so I'm inclined to think perhaps there
> -         is some reason, however perverse.  Perhaps not though...  */
> -      result = fcntl (0, F_SETFL, tinfo->tflags);
>        result = fcntl (0, F_SETFL, tinfo->tflags);
>        OOPSY ("fcntl F_SETFL");
>  #endif
> @@ -403,11 +399,6 @@ child_terminal_ours_1 (int output_only)
>  
>  #ifdef F_GETFL
>        tinfo->tflags = fcntl (0, F_GETFL, 0);
> -
> -      /* Is there a reason this is being done twice?  It happens both
> -         places we use F_SETFL, so I'm inclined to think perhaps there
> -         is some reason, however perverse.  Perhaps not though...  */
> -      result = fcntl (0, F_SETFL, our_terminal_info.tflags);
>        result = fcntl (0, F_SETFL, our_terminal_info.tflags);
>  #endif
>      }
> -- 
> 2.5.5
  

Patch

diff --git a/gdb/inflow.c b/gdb/inflow.c
index a96d4fc..d46d693 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -244,10 +244,6 @@  child_terminal_inferior (struct target_ops *self)
       int result;
 
 #ifdef F_GETFL
-      /* Is there a reason this is being done twice?  It happens both
-         places we use F_SETFL, so I'm inclined to think perhaps there
-         is some reason, however perverse.  Perhaps not though...  */
-      result = fcntl (0, F_SETFL, tinfo->tflags);
       result = fcntl (0, F_SETFL, tinfo->tflags);
       OOPSY ("fcntl F_SETFL");
 #endif
@@ -403,11 +399,6 @@  child_terminal_ours_1 (int output_only)
 
 #ifdef F_GETFL
       tinfo->tflags = fcntl (0, F_GETFL, 0);
-
-      /* Is there a reason this is being done twice?  It happens both
-         places we use F_SETFL, so I'm inclined to think perhaps there
-         is some reason, however perverse.  Perhaps not though...  */
-      result = fcntl (0, F_SETFL, our_terminal_info.tflags);
       result = fcntl (0, F_SETFL, our_terminal_info.tflags);
 #endif
     }