[m-rev.] for review: Let mdb run an external command for 'list'.

[Zoltan Somogyi]

2020-10-02 13:59 GMT+10:00 "Peter Wang" <novalazy at gmail.com>:
> --- a/NEWS
> +++ b/NEWS
> @@ -131,6 +131,14 @@ Changes to the Mercury compiler
>   keep opt1 enabled even if opt1 is not normally enabled at optimization
>   level N.
> +Changes to the Mercury implementation
> +-------------------------------------
> +
> +* The `list` command in mdb (the Mercury debugger) may now call an external
> +  command to print source listings; the command is set using `list_cmd`.
> +  For example, the command could produce syntax highlighted source listings.

Is there  a command that can do so for Mercury code?

> NEWS for Mercury 20.06.1
> ========================
> diff --git a/browser/listing.m b/browser/listing.m
> index c62ff855a..8c79ebd47 100644
> --- a/browser/listing.m
> +++ b/browser/listing.m
> @@ -86,6 +86,21 @@
>     file_name::in, line_no::in, line_no::in, line_no::in, search_path::in,
>     io::di, io::uo) is det.
> +    % list_file_with_command(OutStrm, ErrStrm, FileName, FirstLine, LastLine,
> +    %   MarkLine, Path, !IO):
> +    %
> +    % Like list_file, but invokes an external command to print the source
> +    % listing. The command is passed the four arguments:
> +    %
> +    %   FileName, FirstLine, LastLine, MarkLine
> +    %

Please document MarkLine. And it wouldn't hurt to say
explicitly that FirstLine/LastLine are both inclusive.

> @@ -187,6 +205,15 @@ list_file(OutStrm, ErrStrm, FileName, FirstLine, LastLine, MarkLine, Path,
>         )
>     ).
> +:- func mercury_stream_to_c_FILE_star(io.input_stream) = c_file_ptr.

While the C name of the type may be "FILE *", the Mercury name is c_file_ptr,
so I would s/c_FILE_star/c_file_ptr/ on the function name.

> +            CallResult = error(Error),
> +            write_to_c_file(ErrStrm, "mdb: error running command: ", !IO),
> +            write_to_c_file(ErrStrm, Error, !IO),
> +            write_to_c_file(ErrStrm, "\n", !IO)
> +        )
> +    ;
> +        FindResult = no,
> +        write_to_c_file(ErrStrm, "mdb: cannot find file ", !IO),
> +        write_to_c_file(ErrStrm, FileName, !IO),
> +        write_to_c_file(ErrStrm, "\n", !IO)
> +    ).

I would replace the three calls to write_c_to_file with one call,
preceded by a call to string.format("..."). In both places.

I would also replace the first error message with something along
the lines of "mdb: %s: %s" [s(Command), s(Error)].

> -:- func mercury_stream_to_c_FILE_star(io.input_stream) = c_file_ptr.
> +:- pred find_file(search_path::in, file_name::in, maybe(file_name)::out,
> +    io::di, io::uo) is det.
> -:- pragma foreign_proc("C",
> -    mercury_stream_to_c_FILE_star(InStream::in) = (InStrm::out),
> -    [promise_pure, thread_safe, will_not_call_mercury],
> -"
> -    InStrm = MR_file(*(MR_unwrap_input_stream(InStream)));
> -")

Shouldn't this be next to its almost-identical pair?

> +find_file([], _, no, !IO).
> +find_file([Dir | Path], FileName0, Result, !IO) :-
> +    FileName = Dir / FileName0,
> +    io.check_file_accessibility(FileName, [read], AccessRes, !IO),
> +    (
> +        AccessRes = ok,
> +        Result = yes(FileName)
> +    ;
> +        AccessRes = error(_),
> +        find_file(Path, FileName0, Result, !IO)
> +    ).

Why do you call this only for relative pathnames?

> + at item list_cmd @var{command}
> + at kindex list_cmd (mdb command)
> +Sets an external command to be executed by the @samp{list} command.

I would replace this with:

Tells mdb that all future list commands should be handled by ExternalCommand.

with the name of the argument changed accordingly. (This should reduce confusion,
since there are more than enough commands around.)

> +The command will be called with four arguments:
> +the source file name,
> +the first line number (counting from 1),
> +the last line number,
> +the current line number.
> +The command should print the lines within the range given

all the lines from the first to the last, both inclusive,
with the current line marked (or highlighted) in some fashion

> +to standard output, and report any errors to standard error.
> + at sp 1
> + at item list_cmd
> +When invoked without arguments, the @samp{list_cmd} command
> +prints the last value set by the @samp{list_cmd} command.

If you decide you don't like the selected external command,
is there any way to undo a list_cmd, without restarting mdb?

> --- a/trace/mercury_trace_cmd_parameter.c
> +++ b/trace/mercury_trace_cmd_parameter.c
> @@ -61,6 +61,8 @@ MR_Word                 MR_listing_path;
>  MR_Unsigned             MR_num_context_lines = 2;
> +char                    *MR_listing_cmd = NULL;
> +
> MR_SpyWhen              MR_default_breakpoint_scope = MR_SPY_INTERFACE;
>  ////////////////////////////////////////////////////////////////////////////
> @@ -582,6 +584,34 @@ MR_trace_cmd_pop_list_dir(char **words, int word_count,
>     return KEEP_INTERACTING;
> }
> +MR_Next
> +MR_trace_cmd_list_cmd(char **words, int word_count,
> +    MR_TraceCmdInfo *cmd, MR_EventInfo *event_info, MR_Code **jumpaddr)
> +{
> +    if (word_count == 2) {
> +        char    *copied_value;
> +        char    *aligned_value;
> +
> +        copied_value = (char *) MR_GC_malloc(strlen(words[1]) + 1);
> +        strcpy(copied_value, words[1]);
> +        MR_TRACE_USE_HP(
> +            MR_make_aligned_string(aligned_value, copied_value);
> +        );
> +        MR_listing_cmd = aligned_value;

I would add a test here: if words[1] is "none", then set MR_listing_cmd
to NULL.

> +    } else if (word_count == 1) {
> +        if (MR_listing_cmd != NULL && strlen(MR_listing_cmd) > 0) {
> +            fprintf(MR_mdb_out, "The external listing command is %s\n",
> +                MR_listing_cmd);
> +        } else {
> +            fprintf(MR_mdb_out, "The external listing command has not been set.\n");

I would make this "No external listing command has been set.".

Apart from that, the diff is fine.

Thanks for catching the arg-order-switching bug.

Zoltan.