Re: [PATCH] An implementation of pipe to make I/O communication between gdb and shell.

[Sergio Durigan Junior <sergiodj at redhat dot com>]

Hi Abhijit,

Some minor nits (again).

Abhijit Halder <abhijit.k.halder@gmail.com> writes:

> +struct pipe_obj
> +{
> +  /* The delimiter to separate out gdb-command and shell-command. This can be
                                                                    ^^

Two spaces after the period.

> +  /* The pex object use to create pipeline between gdb and shell.  */

Some typos:

"/*The pex object used to create a pipeline between GDB and shell.  */"

> +  if (*p != '\0') *p++ = '\0';

I'm not found of this style.  Please, put the assignment on the next
line:

        if (*p != '\0')
          *p++ = '\0';


> +  for (pipe->shell_cmd = "";

Sorry, I didn't understand this line.  Is this needed?  If so, I think
it's better if you do `pipe->shell_cmd = NULL'.

> +      int mismatch = memcmp (p, pipe->dlim, (separator-p));

Space between `separator', `-' and `p'.  If you want, you can put this
`memcmp' call directly on the `if' condition, and then you wouldn't need
the braces on the `for'.

> +/* Run execute_command for P and FROM_TTY. Write output to the pipe, do not
                                             ^^
Two spaces after period.

> +  if (pipe->handle == NULL)
> +    error (_("Failed to create pipe"));
> +
> +    {
> +      int status;
> +      const char *err
> +       = pex_run (pipe->pex,
> +		  PEX_SEARCH | PEX_LAST | PEX_STDERR_TO_STDOUT,
> +		  argv[0], argv,
> +		  NULL, NULL,
> +		  &status);
> +      if (err)
> +	error (_("Failed to execute %s"), argv[0]);
> +
> +      do_cleanups (cleanup);
> +    }

Why the braces?

> +  if (pipe->pex)
> +    {
> +      int status;
> +
> +      pex_get_status (pipe->pex, 1, &status);
> +      pex_free (pipe->pex);

Do you really need to call `pex_get_status' here?  I'm really asking,
because I don't know about pex.

Thanks for your work on this patch.

Regards,

Sergio.