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

Sergio Durigan Junior
2011-08-06T22:21:34Z

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.