Re: [PATCH v5 1/5] Move parts of inferior job control to common/

Sergio Durigan Junior
2017-03-31T21:20:52Z

On Friday, March 31 2017, Pedro Alves wrote:

> On 03/31/2017 06:31 PM, Sergio Durigan Junior wrote:
>> On Friday, March 31 2017, Pedro Alves wrote:
>>
>>> On 03/30/2017 02:49 AM, Sergio Durigan Junior wrote:
>>>> gdb/gdbserver/ChangeLog:
>>>> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
>>>>
>>>> * Makefile.in (SFILE): Add "common/job-control.c".
>>>> (OBS): Add "job-control.o".
>>>> * terminal.h: Include "common-terminal.h".
>>>
>>> Is there a reason the .h and the .c files are named differently?
>>
>> Hm, it's just because gdb_setpgid was defined on gdb/terminal.h, so I
>> thought it was better to move it to common-terminal.h (i.e., keep the
>> naming). Do you want a common/job-control.h?
>
> So that .h file contains both definitions of symbols that
> are the exported API of job-control.c, and also the whole
> termios.h/termio.h #ifdefery mess.
>
> It doesn't look like the termios.h stuff is needed by
> the _clients_ of the functions defined in job-control.c?
>
> So I think the best is to have two headers instead.
>
> One with the termios.h/termio.h #ifdef-ery stuff,
> called common/gdb_termios.h. That's be similar in spirit
> to gdb/gdb_curses.h. That should only then be included
> in files that actually need to see those bits defined.
> Maybe job-control.c and inflow.c and not much else.
>
> The "#include <sys/types.h>" include should probably not be
> in the resulting gdb_termios.h. I can't see why it's there
> in your common-terminal.h.
>
> And then add a new common/job-control.h header that matches the
> new job-control.c file, that _only_ exports job-control.c's
> public interface, like, these three declarations:
>
> extern int job_control;
> extern int gdb_setpgid ();
> extern void have_job_control ();

Thanks, it makes sense to separate like this, indeed.

> Also, it looks like gdbserver's terminal.h has all the same
> termios goo that you were putting in common-terminal.h.
> We should remove/undup all that.

Yeah, after a closer look I noticed that the code is just a simplified
version of what's already done on the GDB side (now under common/). So
I went ahead and deleted the gdbserver file.

Thanks for this review, I implemented everything you mentioned on this
message but will wait until you review the other parts before I send the
v6.

Thanks,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible