From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14777 invoked by alias); 18 Jan 2008 18:48:34 -0000 Received: (qmail 14749 invoked by uid 22791); 18 Jan 2008 18:48:32 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 18 Jan 2008 18:48:15 +0000 Received: (qmail 20999 invoked from network); 18 Jan 2008 18:48:13 -0000 Received: from unknown (HELO localhost) (jimb@127.0.0.2) by mail.codesourcery.com with ESMTPA; 18 Jan 2008 18:48:13 -0000 To: "Rob Quill" Cc: msnyder@specifix.com, gdb-patches@sourceware.org Subject: Re: New scope checking patch References: From: Jim Blandy Date: Fri, 18 Jan 2008 18:48:00 -0000 In-Reply-To: (Rob Quill's message of "Fri, 18 Jan 2008 03:35:33 +0000") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.0.50 (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: 2008-01/txt/msg00480.txt.bz2 "Rob Quill" writes: >> > + if (!have_full_symbols () && !have_partial_symbols ()) >> > + error ("No symbol table is loaded. Use the \"file\" command."); >> >> Too much indentation? > > Well, you would think so, but I've changed the tab spacing in vim to 8 > and the way I have formatted it matches how everything else is > formatted. I swear I'm not harrassing you. :) Once we can get these minor things taken care of, the patch looks ready to me. The error should be indented two columns within the 'if'. Here's what I see in most every other use of 'error' in the grammar section of the file. if (new_type == NULL) error ("No type \"%s\" within class or namespace \"%s\".", ncopy, TYPE_NAME (type)); >> Closing brace should get its own line, since the opening brace did. > > This is what I thought too, but if you look at the rest of the file > (for example the case for "exp : exp ARROW name") the open brace > always starts on a new line and always ends on the same line. Right you are. >> When I view this (again, with 8-column tab stops), the 'case' is not >> lined up with the other cases, and the 'return' is not indented two >> spaces within the 'if'. > > Sorry, missed that. I've attached the fixed copy. I believe that the > amount of indentation matches the layout of the current file, and that > even though it looks like a lot in the context of the patch, all the > other grammar rules in the file use the same large amount of > indentation. This code block looks fine to me in your latest patch. The only other point left is the ChangeLog entry. The one you posted was: 2008-01-17 Rob Quill * c-exp.y : Add $in_scope as a type of expression. This needs a little rearrangement, and a bit more detail. A model to follow might be: 2008-01-07 Vladimir Prus Ignore change in name of dynamic linker during execution on Solaris. This also unbreaks pending breakpoints. * solist.h (struct target_so_ops): New field same. * solib-svr4.c (svr4_same): New. (_initialize_svr4_solib): Register svr4_same. * solib.c (update_solib_list): Use ops->same, if available. If the change merits it, it's nice to have a (brief) summary sentence at the top (substantial explanation belongs in comments in the code). Your "Add $in_scope ..." might go there. Then, each change is attributed to a particular function, top-level declaration, or whatever grouping makes sense (in documentation, we put the section name in the parens). So your ChangeLog entry would need to mention IN_SCOPE as a new token you're defining, 'exp' (I guess) as the non-terminal symbol to which you're adding a grammar rule, and 'yylex' as the function to which you're adding a bit of code. 2008-01-17 Rob Quill Add $in_scope as a type of expression. * c-exp.y (IN_SCOPE): ... (exp): ... (yylex): ...