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

[Jan Vrany <jan dot vrany at fit dot cvut dot cz>]

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