From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27573 invoked by alias); 3 Oct 2012 13:57:08 -0000 Received: (qmail 27561 invoked by uid 22791); 3 Oct 2012 13:57:06 -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; Wed, 03 Oct 2012 13:57:03 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id E9D801C75FD; Wed, 3 Oct 2012 09:57:02 -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 954OJiffiCLM; Wed, 3 Oct 2012 09:57:02 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id B57561C75A1; Wed, 3 Oct 2012 09:57:02 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 267C1C67FF; Wed, 3 Oct 2012 15:56:55 +0200 (CEST) Date: Wed, 03 Oct 2012 13:57:00 -0000 From: Joel Brobecker To: Joshua Watt Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Add option to control checking of inner frames when doing a backtrace Message-ID: <20121003135655.GF3028@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.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-10/txt/msg00035.txt.bz2 Hi Joshua, > Please note: this is the first patch I have ever submitted for GDB, so > please let me know if there are any problems. -- Joshua Watt Thanks for sending in a patch. I suggest you read through the gdb/CONTRIBUTE file. It should answer most of the questions. But I still think that it's just better overall to dump the check rather than providing yet another obscure switch which, normally, would require us to test that it actually works as expected. Just for your future contributions, I am including some little notes below, that should help you conform to our project's development style better next time... We're mostly following the GNU Coding Style with some project-specific additions. And one last thing: New commands and settings should be documented in the gdb/NEWS files as well as the GDB manual (gdb/doc/gdb.texinfo). ======================================================================== Aside from missing a ChangeLog entry, GDB's coding style also require that every global variable and function be documented. For instance: > +static unsigned int backtrace_check_inner = TRUE; There should be a comment before backtrace_check_inner's definition: /* Bla bla bla. */ static unsigned int backtrace_check_inner = TRUE; And while looking at this, we typically use int, and zero/nonzero values for this type of usage. If you look at add_setshow_boolean_cmd, you'll see that the value passed is an int*. Similarly: > +static void > +show_backtrace_check_inner (struct ui_file *file, int from_tty, > + struct cmd_list_element *c, const char *value) And there should also be a comment before show_backtrace_check_inner. For functions, we add an empty line between documentation and function. Another small nit: The last line needs to be aligned with the rest of the arguments (using tabs, and then optionally spaces): show_backtrace_check_inner (struct ui_file *file, int from_tty, struct cmd_list_element *c, const char *value) -- Joel