From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9338 invoked by alias); 26 Nov 2010 16:39:00 -0000 Received: (qmail 9226 invoked by uid 22791); 26 Nov 2010 16:38:59 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00 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, 26 Nov 2010 16:38:53 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 161BE2BACB5; Fri, 26 Nov 2010 11:38:52 -0500 (EST) 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 3xu3MrAH9E67; Fri, 26 Nov 2010 11:38:52 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id D83E62BACB4; Fri, 26 Nov 2010 11:38:51 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id CA49F1457E1; Fri, 26 Nov 2010 08:38:42 -0800 (PST) Date: Fri, 26 Nov 2010 16:39:00 -0000 From: Joel Brobecker To: Marc Khouzam Cc: "'gdb-patches@sourceware.org'" Subject: Re: [MI] Duplicate --thread-group flag not detected Message-ID: <20101126163842.GJ2634@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: 2010-11/txt/msg00459.txt.bz2 > 2010-11-25 Marc Khouzam > > * mi/mi-parse.c (mi_parse): Missing else. Normally, the man to approve this is Vladimir, so I can't approve, but I had a quick look anyway. I think that the patch above is correct, given the current implementation. Another very bad scenario: --thread-group i1 --all --frame 1 --thread 1 I think that all the options after --thread-group will be ignored. Speaking of the current implementation: The whole parsing loop looked very odd at first, but I think I'm starting to understand how it works, now. I think it does work right now because there is only 1 option that takes no argument ("--all"). So the code 'swallows' it first if found, and then checks for the other possible switches. It apparently tries to avoid error-handling duplication by putting it at the end of the looping block, but then that leads to: if (*chp != '\0' && !isspace (*chp)) error (_("Invalid value for the '%s' option"), start[2] == 't' ? "--thread" : "--frame"); ... which is missing one case: --thread-group. As the number of options being handled grows (for instance, I have in my TODO an entry for adding support for "--task"), I think that we're going to have to rewrite this slightly differently to make it easier to read and extend... Another remark: It looks like the use of the --all/--thread-group/--thread options with the -mi-break command is not documented? I could only see them documented with commands such as -exec-continue.... Last comment: A small test would be nice... -- Joel