This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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
http://sergiodj.net/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]