From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11734 invoked by alias); 14 Jun 2018 21:40:20 -0000 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 Received: (qmail 11719 invoked by uid 89); 14 Jun 2018 21:40:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=biased, clarification, super, HContent-Transfer-Encoding:8bit X-HELO: mailsec119.isp.belgacom.be Received: from mailsec119.isp.belgacom.be (HELO mailsec119.isp.belgacom.be) (195.238.20.115) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 14 Jun 2018 21:40:16 +0000 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A2CKBwBQ3yJb/+ApQFddHQEBBQELAYNIQ?= =?us-ascii?q?oEfg1pHiGONcjEBkBOGIAsrAYMsgRQCgkgiOBQBAgEBAQEBAQIBbCiCNSQBgk4?= =?us-ascii?q?BAQEDASMPASMzCAMOCgICJgICOR4GAYUtDKl4ghyEXINsgWiBC4kVP4Qbh3aCV?= =?us-ascii?q?QKZDgcCgWqNF404K5EWgVghgVJtgxeQUD2BPQgMAY8hAQE?= X-IPAS-Result: =?us-ascii?q?A2CKBwBQ3yJb/+ApQFddHQEBBQELAYNIQoEfg1pHiGONcjE?= =?us-ascii?q?BkBOGIAsrAYMsgRQCgkgiOBQBAgEBAQEBAQIBbCiCNSQBgk4BAQEDASMPASMzC?= =?us-ascii?q?AMOCgICJgICOR4GAYUtDKl4ghyEXINsgWiBC4kVP4Qbh3aCVQKZDgcCgWqNF40?= =?us-ascii?q?4K5EWgVghgVJtgxeQUD2BPQgMAY8hAQE?= Received: from 224.41-64-87.adsl-dyn.isp.belgacom.be (HELO md) ([87.64.41.224]) by relay.skynet.be with ESMTP/TLS/AES256-GCM-SHA384; 14 Jun 2018 23:40:14 +0200 Message-ID: <1529012414.2365.9.camel@skynet.be> Subject: Re: [RFA_v2 1/8] Add helper functions check_for_flags and check_for_flags_vqcs From: Philippe Waroquiers To: Pedro Alves , gdb-patches@sourceware.org Date: Thu, 14 Jun 2018 21:40:00 -0000 In-Reply-To: References: <20180605204905.30612-1-philippe.waroquiers@skynet.be> <20180605204905.30612-2-philippe.waroquiers@skynet.be> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-06/txt/msg00376.txt.bz2 On Wed, 2018-06-13 at 20:52 +0100, Pedro Alves wrote: > Hi Philippe, > > Been taking a better look at this, finally. Thanks for the review, I will handle all the comments, I have some feedback/questions on a few of them below. > > * cli-utils.c (number_or_range_parser::get_number): Only handle > > numbers or convenience var as numbers. > > (check_for_flags): New function. > > (check_for_flags_vqcs): New function. > > * cli-utils.h (check_for_flags): New function. > > (check_for_flags_vqcs): New function. > > I'm not super happy with this design, because first it is > still at places biased toward "flags" instead of "command > options", and most importantly because it doesn't seem to > make it easy to add further options to commands that > use check_for_flags_vqcs, without the artificial limitation > of requiring all vqcs flags specified together. E.g., what if we want to > add an option like "-foo NUM" to "thread apply" for example. Yes I agree, the current way to give the vqcs option is too unflexible, so I will rework based on the iterative function you suggest below. Just one clarification: I assume that by 'at places biased toward "flags"', you mean that the names should be 'check_for_options_vqcs' ? Otherwise, can you explain what you mean with bias ? > > I'd seem to me that at least an iterative function which > could be interleaved with other options would be better. > Something like: > > struct vqcs_flags > { > /* Number of times -c was seen. */ > int c = 0; > > /* Number of times -v was seen. */ > int v = 0; > }; > > int parse_flags_vqcs (const char **args, vqcs_flags *flags); > > and then > > vqcs_flags flags; > > while (*args != '\0') > { > if (parse_flags_vqcs (&args, &flags)) > continue; > > /* check other options here. error if unknown. */ > } > > maybe even interleave the number-or-range parsing > in that loop. Probably that can be done, but isn't this a little bit cumbersome ? E.g. it means the help thread apply ID... [OPTIONS] COMMAND will become something like thread apply OPTIONS_OR_ID... COMMAND and then we have to explain what OPTIONS_OR_ID can be. (and I guess such syntax might make the 'generalised option parser' more difficult to implement/use : we better keep ID... as a  'positional argument' for an easy conversion to an generalised option/arg parser). So, I am more keen to keep thread apply ID... [OPTIONS] COMMAND (note: we can always change it in a backward compatible way in the future if we really believe mixing OPTIONS and ID... has a strong value). + res = check_for_flags (str, flags, flags_counts); > > + if (res == 0) > > + return 0; > > + if (res == -1) > > + error (_("%s only accepts flags %s given individually"), > > + which_command, flags); > > I think this error message might look a bit odd. What does > it really mean? It catches the below erroneous case, but probably this will become 'unknown option -vc' when I do the iterative design: (gdb) thread apply all -vc p 1 thread apply all only accepts flags vqcs given individually (gdb)  (I think that the generalised option/arg parser you have prototyped will be a nice help to have a consistent and easier to code gdb command parsing :). Thanks Philippe