From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10229 invoked by alias); 24 Dec 2012 04:17:15 -0000 Received: (qmail 10217 invoked by uid 22791); 24 Dec 2012 04:17:14 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 24 Dec 2012 04:17:10 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 4AACF2E05E; Sun, 23 Dec 2012 23:17:10 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id c0OiqhQiTTeM; Sun, 23 Dec 2012 23:17:10 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id CD34B1C7DE6; Sun, 23 Dec 2012 23:17:09 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 8E28CC26BE; Mon, 24 Dec 2012 08:17:01 +0400 (RET) Date: Mon, 24 Dec 2012 04:17:00 -0000 From: Joel Brobecker To: Pierre Muller Cc: gdb-patches@sourceware.org Subject: Re: [RFA] (cli/cli-cmds.c) ARI fix: Avoid assignment inside if statement Message-ID: <20121224041701.GQ5370@adacore.com> References: <001201cde13f$af3ad4b0$0db07e10$@muller@ics-cnrs.unistra.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <001201cde13f$af3ad4b0$0db07e10$@muller@ics-cnrs.unistra.fr> User-Agent: Mutt/1.5.21 (2010-09-15) Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-12/txt/msg00801.txt.bz2 > 2012-12-20 Pierre Muller > > 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