From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27057 invoked by alias); 15 Jun 2012 17:31:33 -0000 Received: (qmail 27047 invoked by uid 22791); 15 Jun 2012 17:31:31 -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; Fri, 15 Jun 2012 17:31:18 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id A69B21C7332; Fri, 15 Jun 2012 13:31:17 -0400 (EDT) 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 AQThmFypoJiJ; Fri, 15 Jun 2012 13:31:17 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 738231C732F; Fri, 15 Jun 2012 13:31:17 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 67D80145616; Fri, 15 Jun 2012 10:31:14 -0700 (PDT) Date: Fri, 15 Jun 2012 17:31:00 -0000 From: Joel Brobecker To: Tristan Gingold Cc: "gdb-patches@sourceware.org ml" Subject: Re: [RFA] Add set/show debug unwind command Message-ID: <20120615173114.GV18729@adacore.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) 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-06/txt/msg00515.txt.bz2 Hi Tristan, > 2012-06-15 Tristan Gingold > > * frame-unwind.c (show_unwind_debug): New function > (unwind_debug): New variable. > (_initialize_frame_unwind): Add show debug unwind command. > * frame-unwind.h (unwind_debug): Declare. Overall, I think this is OK, but since it's not used until your next patch gets in, can you commit both at the same time? You're missing a period at the of the first line. A few comments... > +/* Flag to control debugging. */ > + > +int unwind_debug; > +static void Can you add an empty line between the two? Also, I am really campaining for every single function to be documented, no matter how obvious what the function does. Can you please update this patch accordingly? For the function below, you can just say "Implements the "show debug unwind" command, for instance. > + /* Debug this files internals. */ file's > + add_setshow_zinteger_cmd ("unwind", class_maintenance, &unwind_debug, _("\ > +Set unwind debugging."), _("\ > +Show unwind debugging."), _("\ For such short descriptions, I don't think we should use the formatting you chose. We discussed this a while back, and the consensus was that we should try to avoid it unless it makes sense. Here, I find it does not help reading the code (the contrary, IMO), and generally speaking it screws up the -p option of "diff". > +When non-zero, frame specific internal debugging is enabled."), Even for this one, let's not use that formatting, let's just split it in two strings, like so: _("When non-zero, frame specific internal " "debugging is enabled."), Thanks, -- Joel