From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27910 invoked by alias); 8 Dec 2003 22:49:51 -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 27902 invoked from network); 8 Dec 2003 22:49:50 -0000 Received: from unknown (HELO touchme.toronto.redhat.com) (216.129.200.20) by sources.redhat.com with SMTP; 8 Dec 2003 22:49:50 -0000 Received: from redhat.com (toocool.toronto.redhat.com [172.16.14.72]) by touchme.toronto.redhat.com (Postfix) with ESMTP id 37DAB80044E; Mon, 8 Dec 2003 17:49:50 -0500 (EST) Message-ID: <3FD5000E.4000009@redhat.com> Date: Mon, 08 Dec 2003 22:49:00 -0000 From: "J. Johnston" Organization: Red Hat Inc. User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030624 Netscape/7.1 X-Accept-Language: en-us, en MIME-Version: 1.0 To: David Carlton Cc: gdb-patches@sources.redhat.com Subject: Re: [RFA]: add reason code and silent flag to decode_line_1 References: <3FD4F5D8.4000207@redhat.com> In-Reply-To: Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2003-12/txt/msg00265.txt.bz2 David Carlton wrote: > On Mon, 08 Dec 2003 17:06:16 -0500, "J. Johnston" said: > > >>I have added two new paramters to decode_line_1. One is a >>reason_code field and the other is a silent_flag. The reason code >>is a pointer to an int to store a reason code should the function >>cause an error. At present, the reason_code is only set if the file >>or function is not found. I would have called it not_found_ptr >>instead of reason_code_ptr but I felt this could be enhanced in the >>future to include other forms of error that could be of interest to >>the caller. The reason code works like errno whereby the caller is >>expected to clear the field before calling and check it afterwards. >>The silent_if_not_found flag tells the function not to issue an >>error message if the function is to fail because the function/source >>file is not found. > > > I didn't even know GDB had a function 'throw_exception'; learn > something new every day. Anyways, I'm not in a position to approve > it, but here are my thoughts. My first reaction was "decode_line_1 > has too many arguments to begin with", but that's not your fault. But > I'm not sure you have a coherent story for your two arguments - on the > one hand, you want to be general with reason_code_ptr, but on the > other hand silent_if_not_found is very specialized, and they both > convey overlapping information. > > It seems reasonable to me that you can say "if we fail for certain > reasons, then either we'll convey the reason for failure by calling > error() with an error message or else we'll convey the reason for > failure by storing it in a variable and throwing an exception". I > have a hard time imagining situations where silent_if_not_found is > false but reason_code_ptr is still used, for example. (Assuming, of > course, that further reasons aren't added; and, if they are added, > we'd need to add another silent_if_XXX flag.) > > So I think that you should just add the reason_code_ptr variable (and > probably just call it not_found_ptr for now, though I don't feel > strongly about that), and have the exception be thrown if and only if > it is non-NULL. > I like that solution. I'll resubmit the patch. > (I also think that, if I ever finish my decode_line_1 cleanup, I > should think about reducing the number of its arguments, and I think > that we should switch GDB over to a language with a better exception > model, but those aren't issues that you need to deal with right > now. :-) ) > > A unit test would be nice, too, if possible. > Well, that comes with my pending breakpoint support which I was asked to break into smaller chunks and get the support structure in place first. It will be the first and perhaps only user of this functionality. -- Jeff J.