[m-rev.] for review: Call mdb 'list' external command using posix_spawnp.

[Peter Wang]

On Tue, 06 Oct 2020 01:45:25 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 2020-10-05 16:49 GMT+11:00 "Peter Wang" <novalazy at gmail.com>:
> > +:- pragma foreign_decl("C", local, "
> > +// See library/io.m regarding declartion of the environ global variable.
> 
> Fix spelling.
> 
> > +    rc = posix_spawn_file_actions_init(&file_actions);
> > +    if (rc == -1) {
> > +        *ret_errno = errno;
> > +        goto error_cleanup0;
> > +    }
> 
> I would give the labels error_cleanup[01] names that reflect
> exactly *what* they clean up, since that is less error prone.
> 
Done.

> I don't have any experience with building the action
> descriptors or spawn attribute lists, so I can't effectively check these.
> It should therefore be checked by machine, so you should add
> a test of the mdb "list" command to a test case in tests/debugger.
> (Such a test should have been added earlier, but it seems it wasn't.)

I'll post a test for review.

> > @@ -923,16 +925,22 @@ MR_trace_cmd_list(char **words, int word_count,
> >         MR_make_aligned_string(aligned_filename, (MR_String) filename);
> >     );
> > +    first_lineno = lineno - num;
> > +    last_lineno = lineno + num;
> > +    if (first_lineno < 1) {
> > +        first_lineno = 1;
> > +    }
> > +
> >     if (MR_listing_cmd != NULL && strlen(MR_listing_cmd) > 0) {
> >         MR_TRACE_CALL_MERCURY(
> >             ML_LISTING_list_file_with_command(MR_mdb_out, MR_mdb_err,
> >                 MR_listing_cmd, (char *) aligned_filename,
> > -                lineno - num, lineno + num, lineno, MR_listing_path);
> > +                first_lineno, last_lineno, lineno, MR_listing_path);
> 
> Having a contract between this code and the external list program
> guarantee that first_lineno is never too small but NOT guarantee
> that last_lineno is never too high seems ... odd. I would make
> the contract symmetrical. Is it documented anywhere?

We mention that the first line number starts from 1.

We would need to open and read the file to enforce the high limit,
which is inefficient and unnecessary as the listing implementation would
do that anyway. I think we can expect external commands to silently
ignore limits that are greater than the number of lines in a file.

The problem with a negative line number is that, when passed as an
argument, -INT is likely to be mistaken for an option of some sort.

> 
> Apart from that, the diff is fine.
> 
> Have you considered adding posix_spawn/spawnp to extras/posix?

It's a lot more work to generalise it to be suitable for a library.
I also wouldn't find it useful myself, as I find it more fruitful to
program to low-level C APIs directly within larger foreign code blocks,
rather than (trying to) program C in Mercury syntax.

Peter