From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32759 invoked by alias); 17 Apr 2012 22:41:29 -0000 Received: (qmail 32736 invoked by uid 22791); 17 Apr 2012 22:41:27 -0000 X-SWARE-Spam-Status: No, hits=-5.4 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-vb0-f41.google.com (HELO mail-vb0-f41.google.com) (209.85.212.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 17 Apr 2012 22:40:49 +0000 Received: by vbbey12 with SMTP id ey12so5579566vbb.0 for ; Tue, 17 Apr 2012 15:40:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding:x-system-of-record :x-gm-message-state; bh=3l3+dHeWHJwBRNarhlU2353NLbPImU/JTF0JFckvztg=; b=XAmH+4fy9RB+6ATgoUkZLqATGs8nHZfy8EK87lP2SSRBPSlJpYWhEWezE8VEh5TUe+ qMywMxMmXT0QSSATMJWySyVkaw7Ro3mJtVfF5z1bRri32u5J4EAgCXAo9iY1DBHd4GpK GbM+sWr5XQKO3Wt+caPISX9QAMamg5P2Weqxq/vFQh/J40z+1mdjjPWJoqofybcu4wsG YI7aaVyrJXjtAvPzk8GhdrGykuUOp6mf16rDXjXRAHH0dY6D1lzto0oATZwtjdck9wJi +HDcjweW1akbRbGyw0jPBMR2wOWB8VI2QnRKVEW5oiV0C7XJ1s69CXgV6lsanj0LA+oI az4g== Received: by 10.52.90.20 with SMTP id bs20mr7201489vdb.98.1334702448203; Tue, 17 Apr 2012 15:40:48 -0700 (PDT) MIME-Version: 1.0 Received: by 10.52.90.20 with SMTP id bs20mr7201481vdb.98.1334702447999; Tue, 17 Apr 2012 15:40:47 -0700 (PDT) Received: by 10.220.115.78 with HTTP; Tue, 17 Apr 2012 15:40:47 -0700 (PDT) In-Reply-To: References: Date: Tue, 17 Apr 2012 22:54:00 -0000 Message-ID: Subject: Re: [patch] Create cleanups.[ch] From: Doug Evans To: Sergio Durigan Junior Cc: gdb-patches Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-System-Of-Record: true X-Gm-Message-State: ALoCoQm57igSQbVGzVlQtUVzIm7wpRZ9Z0j+H/TYO42lcG/sE/M1H8Xj3yHuOui5VRINPTgMOifWbDIU6jSMtGNjjbXHUL0luSBc4iQOj320VlAk4On84EBtXBbtxjz406CQqfH8avXY+1Jn1SRWRW2N+pm+AhuvzA== 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/msg00520.txt.bz2 Hi. I didn't want it to seem like I ignored you. :-) Comments inline. On Sun, Apr 15, 2012 at 10:00 PM, Sergio Durigan Junior wrote: > On Sunday, April 15 2012, Doug Evans wrote: > >> Hi. > > Hi Doug. > >> Since cleanups are a big enough source of issues, I want to separate the= m 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. =A0Sorry 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 =A0Doug Evans =A0 >> >> =A0 =A0 =A0 * cleanups.h: New file. >> =A0 =A0 =A0 * cleanups.c: New file. >> =A0 =A0 =A0 * Makefile.in (SFILES): Add cleanups.c >> =A0 =A0 =A0 (HFILES_NO_SRCDIR): Add cleanups.h >> =A0 =A0 =A0 (COMMON_OBS): Add cleanups.o >> =A0 =A0 =A0 * defs.h (struct cleanup): Moved to cleanups.h >> =A0 =A0 =A0 (do_cleanups,do_final_cleanups): Ditto. >> =A0 =A0 =A0 (discard_cleanups,discard_final_cleanups): Ditto >> =A0 =A0 =A0 (make_cleanup,make_cleanup_dtor,make_final_cleanup): Ditto. >> =A0 =A0 =A0 (save_cleanups,save_final_cleanups): Ditto. >> =A0 =A0 =A0 (restore_cleanups,restore_final_cleanups): Ditto. >> =A0 =A0 =A0 (null_cleanup): Ditto. >> =A0 =A0 =A0 (make_my_cleanup,make_my_cleanup2): Delete. >> =A0 =A0 =A0 (discard_my_cleanups,save_my_cleanups,restore_my_cleanups): = Delete. >> =A0 =A0 =A0 * utils.c (cleanup_chain,final_cleanup_chain): Moved to clea= nups.c. >> =A0 =A0 =A0 (do_cleanups,do_final_cleanups): Ditto. >> =A0 =A0 =A0 (discard_cleanups,discard_final_cleanups): Ditto >> =A0 =A0 =A0 (make_cleanup,make_cleanup_dtor,make_final_cleanup): Ditto. >> =A0 =A0 =A0 (save_cleanups,save_final_cleanups): Ditto. >> =A0 =A0 =A0 (restore_cleanups,restore_final_cleanups): Ditto. >> =A0 =A0 =A0 (null_cleanup): Ditto. >> =A0 =A0 =A0 (make_my_cleanup,make_my_cleanup2): Ditto, and make static. >> =A0 =A0 =A0 All uses rewritten to use proper interface. >> =A0 =A0 =A0 (discard_my_cleanups,save_my_cleanups,restore_my_cleanups): = Ditto. > > I remember using `Ditto' once, and being told that I should use > `Likewise' instead. =A0Anyway, I'm just bringing this because I never know > what rule to follow :-). I'm not aware of a specific preference. I probably use either depending on the lunar phase or whatever :-), and there's precedent for both. "Go with the flow." works well on so many levels. >> Index: cleanups.c >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> RCS file: cleanups.c >> diff -N cleanups.c >> --- /dev/null 1 Jan 1970 00:00:00 -0000 >> +++ cleanups.c =A0 =A0 =A0 =A015 Apr 2012 19:23:28 -0000 >> @@ -0,0 +1,210 @@ >> +/* Cleanup routines for GDB, the GNU debugger. >> + >> + =A0 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. I'm of two minds on this, but here I just "went with the flow." >> +#include "defs.h" >> + >> +/* Chain of cleanup actions established with make_cleanup, >> + =A0 to be executed if an error happens. =A0*/ >> + >> +/* Cleaned up after a failed command. =A0*/ >> +static struct cleanup *cleanup_chain; >> + >> +/* Cleaned up when gdb exits. =A0*/ >> +static struct cleanup *final_cleanup_chain; >> + >> +static struct cleanup * >> +make_my_cleanup2 (struct cleanup **pmy_chain, make_cleanup_ftype *funct= ion, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 void *arg, =A0void (*free_arg) (void *)) >> +{ >> + =A0struct cleanup *new >> + =A0 =A0=3D (struct cleanup *) xmalloc (sizeof (struct cleanup)); >> + =A0struct cleanup *old_chain =3D *pmy_chain; >> + >> + =A0new->next =3D *pmy_chain; >> + =A0new->function =3D function; >> + =A0new->free_arg =3D free_arg; >> + =A0new->arg =3D arg; >> + =A0*pmy_chain =3D new; >> + >> + =A0return old_chain; >> +} > > Maybe this function should be named `make_my_cleanup_1', just like other > cases in GDB? =A0I think it should also have a comment here, since it's > static and its declaration/definition is here. Such changes run afoul of the code movement rule. :-) For comments, I went back and added the missing ones though. >> + >> +static struct cleanup * >> +make_my_cleanup (struct cleanup **pmy_chain, make_cleanup_ftype *functi= on, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0void *arg) >> +{ >> + =A0return make_my_cleanup2 (pmy_chain, function, arg, NULL); >> +} > > Comment for this as well. =A0Same thing for all static functions. > >> + >> +/* Add a new cleanup to the cleanup_chain, >> + =A0 and return the previous chain pointer >> + =A0 to be passed later to do_cleanups or discard_cleanups. >> + =A0 Args are FUNCTION to clean up with, and ARG to pass to it. =A0*/ > > 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). =A0Maybe someone more experienced can clarify. I'm not sure there's a documented preference. > I prefer comments in the declaration, FWIW. Me too, though not enough to actually follow it religiously. >> + >> +struct cleanup * >> +make_cleanup (make_cleanup_ftype *function, void *arg) >> +{ >> + =A0return make_my_cleanup (&cleanup_chain, function, arg); >> +} >> + >> +/* Same as make_cleanup except also includes TDOR, a destructor to free= ARG. >> + =A0 DTOR is invoked when the cleanup is performed or when it is discar= ded. =A0*/ >> + >> +struct cleanup * >> +make_cleanup_dtor (make_cleanup_ftype *function, void *arg, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0void (*dtor) (void *)) >> +{ >> + =A0return make_my_cleanup2 (&cleanup_chain, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0function, arg, dtor); > > No need to break the line, I think. Code movement violation. :-) It's easy enough to do a later pass to clean such things up. >> +} >> + >> +static void >> +do_my_cleanups (struct cleanup **pmy_chain, >> + =A0 =A0 =A0 =A0 =A0 =A0 struct cleanup *old_chain) >> +{ >> + =A0struct cleanup *ptr; >> + >> + =A0while ((ptr =3D *pmy_chain) !=3D old_chain) >> + =A0 =A0{ >> + =A0 =A0 =A0*pmy_chain =3D ptr->next; =A0 =A0 =A0 =A0/* Do this first i= n case of recursion. =A0*/ >> + =A0 =A0 =A0(*ptr->function) (ptr->arg); >> + =A0 =A0 =A0if (ptr->free_arg) >> + =A0 =A0 (*ptr->free_arg) (ptr->arg); >> + =A0 =A0 =A0xfree (ptr); >> + =A0 =A0} >> +} > > Comment above the function. =A0Same thing for static functions below. > >> Index: cleanups.h >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> RCS file: cleanups.h >> diff -N cleanups.h >> --- /dev/null 1 Jan 1970 00:00:00 -0000 >> +++ cleanups.h =A0 =A0 =A0 =A015 Apr 2012 19:23:28 -0000 >> @@ -0,0 +1,86 @@ >> +/* Cleanups. >> + =A0 Copyright (C) 1986, 1988-2005, 2007-2012 Free Software Foundation,= Inc. > > Copyright should be 2012. > > -- > Sergio Cheers.