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

[Pedro Alves <palves@redhat.com>]

On 03/31/2017 10:20 PM, Sergio Durigan Junior wrote:
> On Friday, March 31 2017, Pedro Alves wrote:

>> 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.

Note that if you extract the
 gdb/terminal.h / gdbserver/terminal.h
   => common/gdb_termios.h 
part (including the corresponding common.m4 bits) into its own preparatory
patch, and post it separately, we can get that merged and out of the way
quickly.  That part stands on its own right.

Thanks,
Pedro Alves