Re: [PATCH] MI error messages

[Nick Roberts <nickrob at snap dot net dot nz>]

Hi Jason,

 > This patch is changing two things.  You're not using error () when  
 > there is an error, and you're changing the error message texsts.
 > 
 > Why are you avoiding error()?  

I see two, maybe three kinds of error:

1) MI developer
2) Frontend developer
3) User

Type 3 should only be generated, if a debug flag is set perhaps.

Type 2 errors are like:

(gdb)
-var-create
&"mi_cmd_var_create: Usage: NAME FRAME EXPRESSION.\n"
^error,msg="mi_cmd_var_create: Usage: NAME FRAME EXPRESSION."
(gdb) 

The user won't type in -var-create directly, so this error can only be
generated by th frontend and doesn't need log stream output.  Since it
is specific to MI, I don't see why error () need be called.

Type 3 errors are like:

(gdb) 
-stack-select-frame 0
&"mi_cmd_stack_select_frame: No stack.\n"
^error,msg="mi_cmd_stack_select_frame: No stack."
(gdb) 

which in CVS is now reported by:

(gdb) 
-stack-select-frame 0
&"No registers.\n"
^error,msg="No registers."
(gdb) 

It is in the nature of GDB that these errors call error () (through
get_selected_frame).  Actually I would rather this particular situatin was
treated normal, I don't see why "No stack." should flash up as an error
on the status bar if the user opens a window for registers, for example
before execution has started.

 > It's use throughout gdb is established; there are people who like this
 > style of error handling and people who don't, but consistency is valuable
 > and we're using error().  I'm not clear on why you're patching it out of
 > mi-cmd- var.c.

I'm just using mi-cmd- var.c as an example.

 > Is your goal to have the error message output as just ^error w/o the
 > console-quoted error text?

Thats one benefit.  

 > Using your mi_error will result in some error messages (i.e. from the rest
 > of gdb) going through error () and some other error messages (from the mi/
 > sub directory) using mi_error(), so the front end will have both.

When do you see it as appropriate to use MI_CMD_ERROR?

 > Second, the error messages are being changed from e.g.
 > 
 > "mi_cmd_var_create: Usage: NAME FRAME EXPRESSION."
 > 
 > to
 > 
 > "-var-create NAME FRAME EXPRESSION."
 > 
 > Why is this better?

If someone is writing a frontend he might not know anything about the
GDB source code.  If the frontend sends -var-create to GDB without an
argument, he presumably wants to know which MI command generated that
error, not where in the source code it occurred.

 > Neither one means anything to an end user --  

Not to an end user, no, but to a frontend developer.

 > they're just strings for programmers to grep for in the gdb sources,  
 > and to disambiguate the same error message from different mi  
 > commands.

I disagree.

 > Some of the error message changes are good improvements (e.g. adding the
 > symbol that was being var-create'd in "mi_cmd_var_create: unable to create
 > variable object." but you dropped the mi command name, either function or
 > MI command, entirely in that case), but there are a lot of changes in
 > there.

Its for discussion.  I'm not clear what I need to do for Emacs, so I'm making
it up as I go along.  In the three years, I've contributed to Emacs, there
hasn't been a release, so I've not had to deal with compatibility with
existing code before.  Which changes will present problems for you?  In this
situation clearly it doesn't make sense to commit anything until its clear
exactly how it will be used.

 > (I didn't mention the third change, which is to flag mi command usage  
 > error separately, but you can indicate that in the error text already  
 > so I didn't see the compelling reason for that change either...)
 > 
 > We changed error () here at Apple so it is only printed as an MI  
 > ^error msg; our error_stream() reads
 > 
 > 
 >    /* Copy the stream into the GDB_LASTERR buffer.  */
 >    ui_file_rewind (gdb_lasterr);
 >    ui_file_put (stream, do_write, gdb_lasterr);
 > 
 >    /* Write the message plus any error_pre_print to gdb_stderr.
 >       Don't do it for mi like interpreters, however, since
 >       the will get the error from ui_file_rewind. */
 >    if (!ui_out_is_mi_like_p (uiout))
 >      {
 >        target_terminal_ours ();
 >        wrap_here ("");           /* Force out any buffered output */
 >        gdb_flush (gdb_stdout);
 >        annotate_error_begin ();
 >        if (error_pre_print)
 >          fputs_filtered (error_pre_print, gdb_stderr);
 >        ui_file_put (stream, do_write, gdb_stderr);
 >        fprintf_filtered (gdb_stderr, "\n");
 >      }

I thought the line:

      exception_print (gdb_stderr, result);

in  mi_execute_command printed this.  For user errors, isn't the message
in the log stream helpful?  I see now that its not always the same as
the MI error:

-var-create - * value
&"No symbol \"value\" in current context.\n"
&"mi_cmd_var_create: unable to create variable object\n"
^error,msg="mi_cmd_var_create: unable to create variable object"
(gdb) 

 > PS-  If you add "-p" to your $HOME/.cvsrc, e.g.
 > diff -up
 > it will make the diffs a tiny bit easier to read.  -p makes diff try  
 > to figure out the function name and include it in the diff so people  
 > can easily tell which function the change is against.

OK, thanks.

Nick