From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 76189 invoked by alias); 15 Jun 2018 16:25:15 -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 57837 invoked by uid 89); 15 Jun 2018 16:24:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=catches X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 15 Jun 2018 16:24:56 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EB8C0F68AB; Fri, 15 Jun 2018 16:24:27 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4884B42228; Fri, 15 Jun 2018 16:24:27 +0000 (UTC) Subject: Re: [RFA_v2 1/8] Add helper functions check_for_flags and check_for_flags_vqcs To: Philippe Waroquiers , gdb-patches@sourceware.org References: <20180605204905.30612-1-philippe.waroquiers@skynet.be> <20180605204905.30612-2-philippe.waroquiers@skynet.be> <1529012414.2365.9.camel@skynet.be> From: Pedro Alves Message-ID: <0ca18b5e-2b28-f33f-b547-027e00ed17ed@redhat.com> Date: Fri, 15 Jun 2018 16:25:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <1529012414.2365.9.camel@skynet.be> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2018-06/txt/msg00404.txt.bz2 On 06/14/2018 10:40 PM, Philippe Waroquiers wrote: > 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 ? Yeah, on second thought I think using "flags" for the function is fine since it only handles flag-like options. On my option-revamping prototype I was calling those kind of options "switches" but I never like that name. "flags" sounds more usual. So my concern is more with the user-visible aspects. >> maybe even interleave the number-or-range parsing >> in that loop. > Probably that can be done, but isn't this a little bit cumbersome ? Maybe. > 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. I guess we could also say that there's only one "thread apply" command, and that is looks like this: thread apply [OPTION]... [ID... | all] COMMAND and then specifying options after the ID list works just because we don't care about order of optional vs non-optional arguments. But yeah, there's no need to go there. Options after ID is fine with me. > > (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). Agreed. > > + 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 :). I hope so. :-) Thanks, Pedro Alves