On 03/29/2014 08:36 PM, Solly Ross wrote:
> This commit extracts the parsing logic from vshCommnadParse
s/Commnad/Command/
> so that it can be used by other methods. The vshCommnadParse
and again
> method is designed to parse full commands and error out on
> unknown information, so it is not suitable to simply use it
> for autocompletion. Instead, the logic has been extracted.
>
> The new method essentially performs one pass of the loop previously
> done by vshCommnadParse. Depending on what happens, instead of erroring
and again
> or continuing the loop, it returns a value from an enum indicating the
> current state. It also modifieds several arguments as appropriate.
s/modifieds/modifies/
>
> Then, a caller can choose to deal with the state, or may simply ignore
> the state when convinient (vshCommandParse deals with the state,
s/convinient/convenient/
> so as to continue providing its previous functionality, while a
> completer could choose to ingore states involving unknown options,
s/ingore/ignore/
> for example).
> ---
> outgoing/0000-cover-letter.patch | 68 +
> ...prove-virsh-autocompletion-extract-parser.patch | 428 ++++++
> ...prove-virsh-autocompletion-base-framework.patch | 278 ++++
> ...ve-virsh-autocompletion-global-ctl-object.patch | 116 ++
> ...ove-virsh-autocompletion-domain-completer.patch | 1479 ++++++++++++++++++++
> ...virsh-autocompletion-enum-completer-macro.patch | 131 ++
Ouch - these 6 files mistakenly got committed into your tree. They
should not be part of this commit. I'll push a patch to .gitignore to
overlook .patch files, but you'll need to amend your patch to drop these
additions. Easiest might be to use 'git rebase -i', mark this patch as
'edit', then exit the editor to start the efforts. From the shell, I
would then do 'git reset HEAD^' then 'git add tools/virsh.c' then 'git
rebase --continue' - it will preserve your commit message, but should
show just one file instead of 7 in the amended commit.
It's late for me today, so for now, this review is just high-level (I'm
not closely reading the code, just looking for obvious violations of
coding standards).
> diff --git a/tools/virsh.c b/tools/virsh.c
> index f2e4c4a..0273abe 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -1110,7 +1110,7 @@ static vshCmdOptDef helpopt = {
> };
> static const vshCmdOptDef *
> vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name,
> - uint32_t *opts_seen, int *opt_index, char **optstr)
> + uint32_t *opts_seen, int *opt_index, char **optstr, bool raise_err)
Please wrap this line to stay within 80 columns.
> +static vshLineExtractionState
> +vshExtractLinePart(vshControl *ctl, vshCommandParser *parser,
> + uint32_t *opts_seen, uint32_t *opts_need_arg,
> + char **tok_out, const vshCmdDef **cmd,
> + const vshCmdOptDef **opt, bool raise_err, int state)
> +{
> + static bool data_only = false;
> + static uint32_t opts_required = 0;
Explicit initialization of static variables to 0 is overkill; the C
standard already guarantees this. Are these variables really necessary
to be saved across invocations of this function?
> + } else if (tok_type == VSH_TK_SUBCMD_END) {
> + *opt = NULL;
> + //*cmd = NULL;
No // comments, please.
> static bool
> vshCommandParse(vshControl *ctl, vshCommandParser *parser)
> {
> char *tkdata = NULL;
> vshCmd *clast = NULL;
> vshCmdOpt *first = NULL;
> + int state = 0;
Might as well type this with the appropriate enum type, instead of int.
> - /* Special case 'help' to ignore spurious data */
> - if (!(opt = vshCmddefGetData(cmd, &opts_need_arg,
> - &opts_seen)) &&
> - STRNEQ(cmd->name, "help")) {
> - vshError(ctl, _("unexpected data '%s'"), tkdata);
> - goto syntaxError;
> +
> + if (!first)
> + first = arg;
> + if (last)
> + last->next = arg;
> + last = arg;
> +
> + vshDebug(ctl, VSH_ERR_INFO, "%s: %s(%s): %s\n",
> + cmd->name,
> + opt->name,
> + opt->type != VSH_OT_BOOL ? _("optdata") : _("bool"),
> + opt->type != VSH_OT_BOOL ? arg->data : _("(none)"));
> }
> - }
> - if (opt) {
> - /* save option */
> - vshCmdOpt *arg = vshMalloc(ctl, sizeof(vshCmdOpt));
> -
> - arg->def = opt;
> - arg->data = tkdata;
> - arg->next = NULL;
> - tkdata = NULL;
> -
> - if (!first)
> - first = arg;
> - if (last)
> - last->next = arg;
> - last = arg;
> -
> - vshDebug(ctl, VSH_ERR_INFO, "%s: %s(%s): %s\n",
Looks like a lot of reindentation. I find it easier to split patches
like this into two parts - one that is just new content but wrong
indentation, and the other that is just whitespace changes.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list