From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14011 invoked by alias); 5 Dec 2002 17:16:38 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 14002 invoked from network); 5 Dec 2002 17:16:37 -0000 Received: from unknown (HELO mx1.redhat.com) (66.187.233.31) by sources.redhat.com with SMTP; 5 Dec 2002 17:16:37 -0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.11.6/8.11.6) with ESMTP id gB5GpSP09365 for ; Thu, 5 Dec 2002 11:51:28 -0500 Received: from pobox.corp.redhat.com (pobox.corp.redhat.com [172.16.52.156]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id gB5HGaD10041 for ; Thu, 5 Dec 2002 12:16:36 -0500 Received: from localhost.redhat.com (romulus-int.sfbay.redhat.com [172.16.27.46]) by pobox.corp.redhat.com (8.11.6/8.11.6) with ESMTP id gB5HGZ902835; Thu, 5 Dec 2002 12:16:35 -0500 Received: by localhost.redhat.com (Postfix, from userid 469) id 82782FF79; Thu, 5 Dec 2002 12:12:16 -0500 (EST) From: Elena Zannoni MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <15855.35055.794448.770527@localhost.redhat.com> Date: Thu, 05 Dec 2002 09:16:00 -0000 To: David Carlton Cc: Elena Zannoni , gdb-patches@sources.redhat.com Subject: Re: [rfa] linespec.c, part 5 In-Reply-To: References: <15829.39563.373999.459749@localhost.redhat.com> X-SW-Source: 2002-12/txt/msg00152.txt.bz2 David Carlton writes: > On Fri, 15 Nov 2002 20:08:27 -0500, Elena Zannoni said: > > David Carlton writes: > > >> * The function that this creates, decode_compound, is quite long. > >> Later patches in this series will break it down into smaller > >> chunks; they will come after I've got decode_line_1 itself down to > >> a nice size. > > > OK but, could you add something to the comment at the top of the > > function that describes what the function returns and what the > > params are? > > Sure, I can elaborate on that comment. Though I don't think I want to > describe _all_ of the args there: many functions get passed the args > of decode_line_1, so I'd rather put a single comment describing that > somewhere rather than repeating that over and over again. > > Maybe I'll replace the these comments right after the end of > decode_line_1: > > /* Now, still more helper functions. */ > > /* NOTE: carlton/2002-11-07: Some of these have non-obvious side > effects. In particular, if a function is passed ARGPTR as an > argument, it modifies what ARGPTR points to. (Typically, it > advances *ARGPTR past whatever substring it has just looked > at.) */ > > with a comment saying: > > /* Now, more helper functions for decode_line_1. Some conventions > that these functions follow: > > Decode_line_1 typically passes along some of its arguments or local > variables to the subfunctions. It passes the variables by > reference if they are modified by the subfunction, and by value > otherwise. > > Some of the functions have side effects that don't arise from > variables that are passed by reference. In particular, if a > function is passed ARGPTR as an argument, it modifies what ARGPTR > points to; typically, it advances *ARGPTR past whatever substring > it has just looked at. (If it doesn't modify *ARGPTR, then the > function gets passed *ARGPTR instead, which is then called ARG: see > set_flags, for example.) Also, functions that return a struct > symtabs_and_lines may modify CANONICAL, as in the description of > decode_line_1. > > If a function returns a struct symtabs_and_lines, then that struct > will immediately make its way up the call chain to be returned by > decode_line_1. In particular, all of the functions decode_XXX > calculate the appropriate struct symtabs_and_lines, under the > assumption that their argument is of the form XXX. */ > > Is that clearer? > better, yes. > >> * Since decode_line_1 returns the value of decode_compound, there's no > >> need to check whether or not decode_compound modifies the arguments > >> that it's been passed: later code in decode_line_1 can never be > >> effected by such modifications. > > > Hmmm. maybe I am not understanding what you say. Does > > decode_compound modify its args? seems like it changes argptr and > > canonical. What if somebody changes decode_line_1 in the future so > > that there is more code executed after the call, it could probably > > cause some head scratching to see that the call didn't leave things > > as expected. I haven't looked too closely though. > > Does the above comment help? What I meant was: every code path in the > code that I extracted leads to either a return from decode_line_1 or > an error. So, for example, subsequent code in decode_line_1 won't > depend on, say, modifications to 'p' within decode_compound. > > Decode_compound prepares ARGPTR and CANONICAL appropriately for > return, and calculates the struct symtabs_and_lines to be returned. > If somebody subsequently decides that, say, a bit more fiddling should > happen within decode_line_1, I think these changes should make that > fiddling a lot easier rather than a lot harder. > I guess we'll heve to wait for the rest of your patches. If that's still an issue we can revisit later. > It will also help once I've renamed some variables, so 's' turns into > 'file_symtab', 'p' turns into 'next_component', and so forth. That > will make it much easier for future readers of the code to understand > the possible ramifications of changing the values of those variables. > (Whereas who knows what it means to change the value of a variable 'p' > that is referred to in a zillion places, with some of those uses > distinct and some of them not.) > BTW, I did a diff -w of decode_compound and the code you removed, and I noticed this (ignore the line numbers): @@ -211,7 +231,6 @@ *argptr = (*p == '\'') ? p + 1 : p; /* Look up entire name */ sym = lookup_symbol (copy, 0, VAR_NAMESPACE, 0, &sym_symtab); - s = (struct symtab *) 0; if (sym) return symbol_found (funfirstline, canonical, copy, sym, NULL, sym_symtab); I agree with you that such assignment didn't make much sense. Dig dig dig, it came in with the HP merge. It was doing this: /* Look up entire name */ sym = lookup_symbol (copy, 0, VAR_NAMESPACE, 0, &sym_symtab); s = (struct symtab *) 0; /* Prepare to jump: restore the " if (condition)" so outer layers see it */ if (has_if) *ii = ' '; /* Symbol was found --> jump to normal symbol processing. Code following "symbol_found" expects "copy" to have the symbol name, "sym" to have the symbol pointer, "s" to be a specified file's symtab, and sym_symtab to be the symbol's symtab. */ /* By jumping there we avoid falling through the FILE:LINE and FILE:FUNC processing stuff below */ if (sym) goto symbol_found; and symbol_found: was (is) checking for s==0. But now that would be the NULL parameter in the symbol_found call right after. So I think that's safe. OK check it in. Elena