[RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Pierre Muller]

  Looks almost obvious, but
as it's the first of that kind,
I thought I will send it as RFA nonetheless...
  
  Can similar changes be committed as obvious?
They usually require some formatting changes,
but generated behavior should not change.


Pierre Muller
as ARI maintainer


2012-12-20  Pierre Muller  <muller@sourceware.org>

        ARI fixes: Assignment within if rule.
        * cli/cli-cmds.c (shell_escape): Do not set variable value inside
        if statement.
        (edit_command): Likewise.

Index: src/gdb/cli/cli-cmds.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-cmds.c,v
retrieving revision 1.143
diff -u -p -r1.143 cli-cmds.c
--- src/gdb/cli/cli-cmds.c      18 Dec 2012 19:27:35 -0000      1.143
+++ src/gdb/cli/cli-cmds.c      20 Dec 2012 16:10:27 -0000
@@ -724,11 +724,13 @@ shell_escape (char *arg, int from_tty)
 #else /* Can fork.  */
   int status, pid;

-  if ((pid = vfork ()) == 0)
+  pid = vfork ();
+  if (pid == 0)
     {
       const char *p, *user_shell;

-      if ((user_shell = (char *) getenv ("SHELL")) == NULL)
+      user_shell = (char *) getenv ("SHELL");
+      if (user_shell == NULL)
        user_shell = "/bin/sh";

       /* Get the name of the shell for arg0.  */
@@ -837,7 +839,8 @@ edit_command (char *arg, int from_tty)
         error (_("No line number known for %s."), arg);
     }

-  if ((editor = (char *) getenv ("EDITOR")) == NULL)
+  editor = (char *) getenv ("EDITOR");
+  if (editor == NULL)
       editor = "/bin/ex";

   /* If we don't already know the full absolute file name of the

Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Joel Brobecker]

> 2012-12-20  Pierre Muller  <muller@sourceware.org>
> 
>         ARI fixes: Assignment within if rule.
>         * cli/cli-cmds.c (shell_escape): Do not set variable value inside
>         if statement.
>         (edit_command): Likewise.

Thanks for doing this, Pierre!

Aside from the meta-discussion on the rule itself, does anyone
see a problem with this patch? I see it as a clear improvement,
so I would like to approve it.

One tiny comment below...

> Index: src/gdb/cli/cli-cmds.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/cli/cli-cmds.c,v
> retrieving revision 1.143
> diff -u -p -r1.143 cli-cmds.c
> --- src/gdb/cli/cli-cmds.c      18 Dec 2012 19:27:35 -0000      1.143
> +++ src/gdb/cli/cli-cmds.c      20 Dec 2012 16:10:27 -0000
> @@ -724,11 +724,13 @@ shell_escape (char *arg, int from_tty)
>  #else /* Can fork.  */
>    int status, pid;
> 
> -  if ((pid = vfork ()) == 0)
> +  pid = vfork ();
> +  if (pid == 0)
>      {
>        const char *p, *user_shell;
> 
> -      if ((user_shell = (char *) getenv ("SHELL")) == NULL)
> +      user_shell = (char *) getenv ("SHELL");
> +      if (user_shell == NULL)
>         user_shell = "/bin/sh";
> 
>        /* Get the name of the shell for arg0.  */
> @@ -837,7 +839,8 @@ edit_command (char *arg, int from_tty)
>          error (_("No line number known for %s."), arg);
>      }
> 
> -  if ((editor = (char *) getenv ("EDITOR")) == NULL)
> +  editor = (char *) getenv ("EDITOR");

According to POSIX, getenv returns char *, so I do not think
that the cast is necessary. Let's try without and see what happens...

> +  if (editor == NULL)
>        editor = "/bin/ex";
> 
>    /* If we don't already know the full absolute file name of the

-- 
Joel

Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Andreas Schwab]

Joel Brobecker <brobecker@adacore.com> writes:

>> Index: src/gdb/cli/cli-cmds.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/cli/cli-cmds.c,v
>> retrieving revision 1.143
>> diff -u -p -r1.143 cli-cmds.c
>> --- src/gdb/cli/cli-cmds.c      18 Dec 2012 19:27:35 -0000      1.143
>> +++ src/gdb/cli/cli-cmds.c      20 Dec 2012 16:10:27 -0000
>> @@ -724,11 +724,13 @@ shell_escape (char *arg, int from_tty)
>>  #else /* Can fork.  */
>>    int status, pid;
>> 
>> -  if ((pid = vfork ()) == 0)
>> +  pid = vfork ();
>> +  if (pid == 0)
>>      {
>>        const char *p, *user_shell;
>> 
>> -      if ((user_shell = (char *) getenv ("SHELL")) == NULL)
>> +      user_shell = (char *) getenv ("SHELL");
>> +      if (user_shell == NULL)
>>         user_shell = "/bin/sh";
>> 
>>        /* Get the name of the shell for arg0.  */
>> @@ -837,7 +839,8 @@ edit_command (char *arg, int from_tty)
>>          error (_("No line number known for %s."), arg);
>>      }
>> 
>> -  if ((editor = (char *) getenv ("EDITOR")) == NULL)
>> +  editor = (char *) getenv ("EDITOR");
>
> According to POSIX, getenv returns char *, so I do not think
> that the cast is necessary. Let's try without and see what happens...

Even more so as editor should really be const char * (and user_shell
already is).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Eli Zaretskii]

> From: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>
> Date: Sun, 23 Dec 2012 19:59:49 +0100
> 
>   Looks almost obvious, but
> as it's the first of that kind,
> I thought I will send it as RFA nonetheless...
>   
>   Can similar changes be committed as obvious?
> They usually require some formatting changes,
> but generated behavior should not change.
> 
> 
> Pierre Muller
> as ARI maintainer
> 
> 
> 2012-12-20  Pierre Muller  <muller@sourceware.org>
> 
>         ARI fixes: Assignment within if rule.
>         * cli/cli-cmds.c (shell_escape): Do not set variable value inside
>         if statement.
>         (edit_command): Likewise.

Since when is that bad C, so much so that we would need to enforce it?

RE: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Eli Zaretskii
> Envoyé : dimanche 23 décembre 2012 20:29
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if
> statement
> 
> > From: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>
> > Date: Sun, 23 Dec 2012 19:59:49 +0100
> >
> >   Looks almost obvious, but
> > as it's the first of that kind,
> > I thought I will send it as RFA nonetheless...
> >
> >   Can similar changes be committed as obvious?
> > They usually require some formatting changes,
> > but generated behavior should not change.
> >
> >
> > Pierre Muller
> > as ARI maintainer
> >
> >
> > 2012-12-20  Pierre Muller  <muller@sourceware.org>
> >
> >         ARI fixes: Assignment within if rule.
> >         * cli/cli-cmds.c (shell_escape): Do not set variable value
inside
> >         if statement.
> >         (edit_command): Likewise.
> 
> Since when is that bad C, so much so that we would need to enforce it?

Eli, as you know, 
I am not a C specialist at all...

But I didn't invent that rule either.
This rule part of gdb_ari.sh since it's introduction in CVS ss repository
back in 2002.

 It's probably now your chance to challenge this rule...
I would tend to agree that an assignment should be a statement on it's 
own, but you could rightly consider this as a 'pascal' language
preference!


Pierre Muller
both ARI and pascal language support maintainer!

Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> Since when is that bad C, so much so that we would need to enforce it?

It's part of the GNU coding standards.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>,  gdb-patches@sourceware.org
> Date: Sun, 23 Dec 2012 23:28:45 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Since when is that bad C, so much so that we would need to enforce it?
> 
> It's part of the GNU coding standards.

Which says "Try not to...".

FWIW, Emacs sources are replete with such assignments, and I don't see
anything wrong with that in the first place.

So yes, I guess I'm challenging this ARI rule.

Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> FWIW, Emacs sources are replete with such assignments,

Yes, they should be avoided.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

RE: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Pierre Muller]

  Hi all,

   I would like to know what the status of this patch is,
and more generally of the ARI rule: "Avoid assignment inside if statement".

  Eli wanted to challenge this rule,
but others seems to be in favor...

  I really do find code easier to read if assignments are done
before the if statement, but this is due to my pascal background,
which forbids it.

 
  Pierre Muller,
as ARI maintainer.
  


> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Eli Zaretskii
> Envoyé : lundi 24 décembre 2012 04:41
> À : Andreas Schwab
> Cc : pierre.muller@ics-cnrs.unistra.fr; gdb-patches@sourceware.org
> Objet : Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if
> statement
> 
> > From: Andreas Schwab <schwab@linux-m68k.org>
> > Cc: Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>,  gdb-
> patches@sourceware.org
> > Date: Sun, 23 Dec 2012 23:28:45 +0100
> >
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> > > Since when is that bad C, so much so that we would need to enforce it?
> >
> > It's part of the GNU coding standards.
> 
> Which says "Try not to...".
> 
> FWIW, Emacs sources are replete with such assignments, and I don't see
> anything wrong with that in the first place.
> 
> So yes, I guess I'm challenging this ARI rule.

Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Joel Brobecker]

>    I would like to know what the status of this patch is,
> and more generally of the ARI rule: "Avoid assignment inside if statement".

I think that we should continue to discourage the use of assignments
inside if statements, and the ARI is a good tool for that. If someone
happens to come across a situation where it is clearly advantageous
to break this rule, then we can document it using an ARI comment
that clearly allows it.

-- 
Joel

Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Sergio Durigan Junior]

On Sunday, December 23 2012, Eli Zaretskii wrote:

>> From: "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr>
>> Date: Sun, 23 Dec 2012 19:59:49 +0100
>> 
>>   Looks almost obvious, but
>> as it's the first of that kind,
>> I thought I will send it as RFA nonetheless...
>>   
>>   Can similar changes be committed as obvious?
>> They usually require some formatting changes,
>> but generated behavior should not change.
>> 
>> 
>> Pierre Muller
>> as ARI maintainer
>> 
>> 
>> 2012-12-20  Pierre Muller  <muller@sourceware.org>
>> 
>>         ARI fixes: Assignment within if rule.
>>         * cli/cli-cmds.c (shell_escape): Do not set variable value inside
>>         if statement.
>>         (edit_command): Likewise.
>
> Since when is that bad C, so much so that we would need to enforce it?

Interesting...  I remember when I started hacking GDB, I was strongly
discouraged to do assignments inside `if' checkings.  I don't remember
who told me that, but the reason was something related to the Coding
Standards (I don't have any references either).  Anyway, since that
moment I stopped doing this...

FWIW, I agree that it is not bad C, and totally valid, so I for one vote
to remove this rule from ARI.  Nevertheless, I will take a closer look
at the Coding Standards to make sure we're not doing something "wrong"
in this case...

-- 
Sergio

Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement

[Joel Brobecker]

> > Since when is that bad C, so much so that we would need to enforce it?
> 
> Interesting...  I remember when I started hacking GDB, I was strongly
> discouraged to do assignments inside `if' checkings.  I don't remember
> who told me that, but the reason was something related to the Coding
> Standards (I don't have any references either).  Anyway, since that
> moment I stopped doing this...

It is in the GNU Coding Standards:
http://www.gnu.org/prep/standards/standards.html#Syntactic-Conventions

In particular:

    | Try to avoid assignments inside if-conditions (assignments inside
    | while-conditions are ok). For example, don’t write this:
    |
    | if ((foo = (char *) malloc (sizeof *foo)) == 0)
    |   fatal ("virtual memory exhausted");
    | instead, write this:
    |
    | foo = (char *) malloc (sizeof *foo);
    | if (foo == 0)
    |   fatal ("virtual memory exhausted");

FWIW, I mostly agree with this suggestion, particulary in the
examples above. I do remember seeing some examples where it was
more practical if we could have assignments inside conditions
(in an "if ... else if ..." situation), but even then, I find
that it makes it harder to notice the assignment.

In the examples that Pierre chose to fix, it seems obvious to me
(this is a judgement call, so potentially subject to disagreement)
that there was no need to have the assignment inside the condition,
and that the patched code is easier to grasp. I think it is
a clear improvement.

> FWIW, I agree that it is not bad C, and totally valid

I think that the point is not that this is bad C, or not
valid/portable. I think that the point is that it is judged
to be a poor practice.

-- 
Joel