From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10458 invoked by alias); 5 May 2010 21:41:30 -0000 Received: (qmail 10445 invoked by uid 22791); 5 May 2010 21:41:28 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 05 May 2010 21:41:23 +0000 Received: (qmail 5246 invoked from network); 5 May 2010 21:41:22 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 5 May 2010 21:41:22 -0000 From: Pedro Alves To: Michael Snyder Subject: Re: [ob] remote.c, eliminate unused variables Date: Wed, 05 May 2010 21:41:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-20-generic; KDE/4.3.2; x86_64; ; ) Cc: "gdb-patches@sourceware.org" References: <4BE1D93F.8000309@vmware.com> <201005052151.19745.pedro@codesourcery.com> <4BE1DFA9.9060607@vmware.com> In-Reply-To: <4BE1DFA9.9060607@vmware.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201005052241.14753.pedro@codesourcery.com> X-IsSubscribed: yes 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: 2010-05/txt/msg00130.txt.bz2 On Wednesday 05 May 2010 22:14:17, Michael Snyder wrote: > Pedro Alves wrote: > > On Wednesday 05 May 2010 21:46:55, Michael Snyder wrote: > >> --- remote.c 5 May 2010 15:05:57 -0000 1.405 > >> +++ remote.c 5 May 2010 20:43:46 -0000 > >> @@ -2512,8 +2512,8 @@ remote_threads_info (struct target_ops * > >> { > >> struct gdb_xml_parser *parser; > >> struct threads_parsing_context context; > >> - struct cleanup *back_to = make_cleanup (null_cleanup, NULL); > >> > >> + make_cleanup (null_cleanup, NULL); > > > > Are you making sure (in all your patches) that the reason the > > variables are unused isn't itself a bug? > > Can't guarantee it, no. > I'm making sure the semantics isn't changed, but I can't always > be sure that the original semantics was right. Well, then I'll ask please, don't "fix" more things like this, and surely don't call it obvious. You're removing a warning for the sake of it. A warning is useful as a hint at something wrong with the code; there may be something genuinely wrong with it. Removing it blindly removes the useful hint. If you want to be bothered to look at the code to see if there's something else genuinely wrong, then please, don't change it. > > In this case, creating a > > null_cleanup and not storing a pointer anywhere is > > highly suspicious... > > Well, then it was wrong when I got there. The variable that > it was stored to was not used, but shadowed an outer scope variable > of the same name, which was used. > > Maybe it should have stored it without declaring it? Store what? What do you mean? > I don't know... what do you think? null_cleanup's only serve one single purpose --- to introduce a new cleanup scope, so you can do things like: struct cleanup *old_chain = make_cleanup (null_cleanup, NULL); ... foo = xmalloc (big_chunk); make_cleanup (xfree, foo); ... bar = xmalloc (big_chunk2); make_cleanup (foo, bar); ... baz = xmalloc (big_chunk3); make_cleanup (bar, baz); ... do_cleanup (old_chain); Note no pointer is stored for the intervening cleanups. In this particular case, in remote.c, there's no other cleanup created in the scope in question, and the outer scope already has a proper do_cleanups, so the right thing to do is to remove the whole `make_cleanups (null_cleanup, NULL)' line. But, and here's the genuine bug, if gdb_xml_parse throws midway while parsing the xml, there's no cleanup set to release `context.items'. There's an usual pattern around all these `gdb_xml_create_parser_and_cleanup' calls that this particular case isn't following (and the cleanup in question may indicate that it was simply forgotten, and so the warning was being useful). -- Pedro Alves