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

Sergio Durigan Junior
2017-11-02T18:34:19Z

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

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible