Re: [PATCH] MI: Add new command -complete

Jan Vrany
2019-01-17T21:01:32Z

On Thu, 2019-01-17 at 13:29 -0700, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:
>
> Jan> This commit adds a new '-complete' MI command which is implemented using
> Jan> CLI's 'complete'.
>
> I suspect it would be better to just write a new MI function and then
> mildly refactor complete_command so that the two commands can share most
> of their implementation.
>
> First, this would make it so we could add options to the MI command as
> needed. (Perhaps the current argument to the command ought to have a
> flag to avoid future parsing problems?

What do you mean by this? Can you give an example?

> I am not sure.)
>
> Second, it would mean that we could make changes to the CLI command
> without worrying about breaking MI compatibility.

As for changes to CLI command, I've got the impression from the code that
the format is rather strict (from use of printf_unfiltered(), for instance),
deliberately so to not break clients (Emacs is mentioned). Indeed, not really
an argument against splitting.

>
> What do you think of this?

I thought of that actually. Also, it'd make the MI interface more regular
w.r.t quoting of spaces and other special characters. But in the end I could
not find a way without duplicating enough code from the original function,
so hacked it that way.

Let me try harder :-)

>
> I looked and I see that some other MI commands are implemented directly
> by CLI commands. I am not sure but I suspect this might be a deprecated
> approach.

Yeah, there's quite a lot of them. If it is a deprecated approach,
I did not know.

>
> Jan> + bool is_mi_like = uiout->is_mi_like_p ();
>
> This could be removed by having a more explicit API.

I'm sorry, I'm not sure what do you mean here.

>
> Jan> + ui_out_emit_list completions_emitter (uiout, "completions");
> Jan> + if (result.number_matches != 0)
> Jan> + {
> Jan> + if (result.number_matches == 1)
> Jan> + {
> Jan> + if (is_mi_like)
> Jan> + uiout->field_fmt(NULL, "%s%s", arg_prefix.c_str (),
> Jan> + result.match_list[0]);
> Jan> + else
> Jan> + printf_unfiltered ("%s%s\n", arg_prefix.c_str (),
> Jan> + result.match_list[0]);
>
> Space before "(" in the field_fmt call. Unifying the cases would be
> preferable. Or another argument for separation.
>

Thanks!
Jan