From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15774 invoked by alias); 16 Apr 2012 05:01:07 -0000 Received: (qmail 15763 invoked by uid 22791); 16 Apr 2012 05:01:05 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 16 Apr 2012 05:00:45 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q3G50i9L004543 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 16 Apr 2012 01:00:44 -0400 Received: from psique (ovpn-112-22.phx2.redhat.com [10.3.112.22]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q3G50der009317; Mon, 16 Apr 2012 01:00:42 -0400 From: Sergio Durigan Junior To: Doug Evans Cc: gdb-patches@sourceware.org Subject: Re: [patch] Create cleanups.[ch] References: X-URL: http://www.redhat.com Date: Mon, 16 Apr 2012 07:27:00 -0000 In-Reply-To: (Doug Evans's message of "Sun, 15 Apr 2012 12:42:37 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: 2012-04/txt/msg00399.txt.bz2 On Sunday, April 15 2012, Doug Evans wrote: > Hi. Hi Doug. > Since cleanups are a big enough source of issues, I want to separate them out. > This patch moves the core cleanup API into its own file. > It makes no other changes. > I have at least one more patch to go, but I want to get this done > first. Thanks, I'm always in favor of such API separations. Sorry for nitpicking, I know you are just moving the code around, but since you touched it I felt I should take a look even if it's old code (maybe, *especially* because of that!). > 2012-04-15 Doug Evans > > * cleanups.h: New file. > * cleanups.c: New file. > * Makefile.in (SFILES): Add cleanups.c > (HFILES_NO_SRCDIR): Add cleanups.h > (COMMON_OBS): Add cleanups.o > * defs.h (struct cleanup): Moved to cleanups.h > (do_cleanups,do_final_cleanups): Ditto. > (discard_cleanups,discard_final_cleanups): Ditto > (make_cleanup,make_cleanup_dtor,make_final_cleanup): Ditto. > (save_cleanups,save_final_cleanups): Ditto. > (restore_cleanups,restore_final_cleanups): Ditto. > (null_cleanup): Ditto. > (make_my_cleanup,make_my_cleanup2): Delete. > (discard_my_cleanups,save_my_cleanups,restore_my_cleanups): Delete. > * utils.c (cleanup_chain,final_cleanup_chain): Moved to cleanups.c. > (do_cleanups,do_final_cleanups): Ditto. > (discard_cleanups,discard_final_cleanups): Ditto > (make_cleanup,make_cleanup_dtor,make_final_cleanup): Ditto. > (save_cleanups,save_final_cleanups): Ditto. > (restore_cleanups,restore_final_cleanups): Ditto. > (null_cleanup): Ditto. > (make_my_cleanup,make_my_cleanup2): Ditto, and make static. > All uses rewritten to use proper interface. > (discard_my_cleanups,save_my_cleanups,restore_my_cleanups): Ditto. I remember using `Ditto' once, and being told that I should use `Likewise' instead. Anyway, I'm just bringing this because I never know what rule to follow :-). > Index: cleanups.c > =================================================================== > RCS file: cleanups.c > diff -N cleanups.c > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ cleanups.c 15 Apr 2012 19:23:28 -0000 > @@ -0,0 +1,210 @@ > +/* Cleanup routines for GDB, the GNU debugger. > + > + Copyright (C) 1986, 1988-2012 Free Software Foundation, Inc. It should be: Copyright (C) 2012 Free Software Foundation, Inc. AFAIK, since it's a new file. > +#include "defs.h" > + > +/* Chain of cleanup actions established with make_cleanup, > + to be executed if an error happens. */ > + > +/* Cleaned up after a failed command. */ > +static struct cleanup *cleanup_chain; > + > +/* Cleaned up when gdb exits. */ > +static struct cleanup *final_cleanup_chain; > + > +static struct cleanup * > +make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *function, > + void *arg, void (*free_arg) (void *)) > +{ > + struct cleanup *new > + = (struct cleanup *) xmalloc (sizeof (struct cleanup)); > + struct cleanup *old_chain = *pmy_chain; > + > + new->next = *pmy_chain; > + new->function = function; > + new->free_arg = free_arg; > + new->arg = arg; > + *pmy_chain = new; > + > + return old_chain; > +} Maybe this function should be named `make_my_cleanup_1', just like other cases in GDB? I think it should also have a comment here, since it's static and its declaration/definition is here. > + > +static struct cleanup * > +make_my_cleanup (struct cleanup **pmy_chain, make_cleanup_ftype *function, > + void *arg) > +{ > + return make_my_cleanup2 (pmy_chain, function, arg, NULL); > +} Comment for this as well. Same thing for all static functions. > + > +/* Add a new cleanup to the cleanup_chain, > + and return the previous chain pointer > + to be passed later to do_cleanups or discard_cleanups. > + Args are FUNCTION to clean up with, and ARG to pass to it. */ Maybe these comments can be made to fill more than 42 characters in the line? I also never know what's the best/recommended practice: to put the comments above the function's declaration (in this case, in the cleanups.h file), or to put comments above the function definition (as you did). Maybe someone more experienced can clarify. I prefer comments in the declaration, FWIW. > + > +struct cleanup * > +make_cleanup (make_cleanup_ftype *function, void *arg) > +{ > + return make_my_cleanup (&cleanup_chain, function, arg); > +} > + > +/* Same as make_cleanup except also includes TDOR, a destructor to free ARG. > + DTOR is invoked when the cleanup is performed or when it is discarded. */ > + > +struct cleanup * > +make_cleanup_dtor (make_cleanup_ftype *function, void *arg, > + void (*dtor) (void *)) > +{ > + return make_my_cleanup2 (&cleanup_chain, > + function, arg, dtor); No need to break the line, I think. > +} > + > +static void > +do_my_cleanups (struct cleanup **pmy_chain, > + struct cleanup *old_chain) > +{ > + struct cleanup *ptr; > + > + while ((ptr = *pmy_chain) != old_chain) > + { > + *pmy_chain = ptr->next; /* Do this first in case of recursion. */ > + (*ptr->function) (ptr->arg); > + if (ptr->free_arg) > + (*ptr->free_arg) (ptr->arg); > + xfree (ptr); > + } > +} Comment above the function. Same thing for static functions below. > Index: cleanups.h > =================================================================== > RCS file: cleanups.h > diff -N cleanups.h > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ cleanups.h 15 Apr 2012 19:23:28 -0000 > @@ -0,0 +1,86 @@ > +/* Cleanups. > + Copyright (C) 1986, 1988-2005, 2007-2012 Free Software Foundation, Inc. Copyright should be 2012. -- Sergio