Re: [Qemu-devel] [PATCH for 2.13 1/5] linux-user: cleanup signal.c

[Peter Maydell]

On 23 March 2018 at 16:51, Laurent Vivier <address@hidden> wrote:
> Le 23/03/2018 à 15:22, Peter Maydell a écrit :
>> On 22 March 2018 at 21:58, Laurent Vivier <address@hidden> wrote:
>>> move all target specific parts to
>>> signal.inc.c in arch directory
>>>
>>> Signed-off-by: Laurent Vivier <address@hidden>
>>> ---
>>>  linux-user/aarch64/signal.inc.c    |  556 +++
>>>  linux-user/alpha/signal.inc.c      |  258 ++
>>>  linux-user/arm/signal.inc.c        |  749 +++++
>>>  linux-user/cris/signal.inc.c       |  166 +
>>>  linux-user/hppa/signal.inc.c       |  188 ++
>>>  linux-user/i386/signal.inc.c       |  579 ++++
>>>  linux-user/m68k/signal.inc.c       |  406 +++
>>>  linux-user/microblaze/signal.inc.c |  226 ++
>>>  linux-user/mips/signal.inc.c       |  378 +++
>>>  linux-user/mips64/signal.inc.c     |    1 +
>>>  linux-user/nios2/signal.inc.c      |  232 ++
>>>  linux-user/openrisc/signal.inc.c   |  209 ++
>>>  linux-user/ppc/signal.inc.c        |  667 ++++
>>>  linux-user/riscv/signal.inc.c      |  196 ++
>>>  linux-user/s390x/signal.inc.c      |  305 ++
>>>  linux-user/sh4/signal.inc.c        |  327 ++
>>>  linux-user/signal.c                | 6487 
>>> +-----------------------------------
>>>  linux-user/sparc/signal.inc.c      |  601 ++++
>>>  linux-user/sparc64/signal.inc.c    |    1 +
>>>  linux-user/tilegx/signal.inc.c     |  163 +
>>>  linux-user/x86_64/signal.inc.c     |    1 +
>>>  linux-user/xtensa/signal.inc.c     |  253 ++
>>>  22 files changed, 6463 insertions(+), 6486 deletions(-)
>>
>> I think anything with a diffstat like this is basically
>> unreviewable, at least for me :-(
>>
>>> diff --git a/linux-user/aarch64/signal.inc.c 
>>> b/linux-user/aarch64/signal.inc.c
>>> new file mode 100644
>>> index 0000000000..28fa0f2f22
>>> --- /dev/null
>>> +++ b/linux-user/aarch64/signal.inc.c
>>
>> I was hoping we could have these be standalone .c files,
>> rather than just #included from linux-user/signal.c.
>> I appreciate this requires a bit more teasing out of what
>> needs to become non-static in the common code signal.c
>> to be callable from the per-target code, though.
>>
>
> Thank you for your review.
>
> I tried the easy way first... but I'm going to try to do standalone c files.

Structuring your patchset so it can move one architecture
at a time out ouf the common file will probably help
in making the patch more reviewable, though it's likely
a bit more painful to do.

I think at the time I was looking at this I was contemplating
an approach like:
 (1) for architecture A, move that arch's code from signal.c
  to $ARCH/signal.c, and temporarily use a #include to
  include it from signal.c
 (2) repeat for architectures B, C, ..., one per patch
 (3) make the makefile and code changes needed to
  drop the #includes and build $ARCH/signal.c as
  standalone files, as a final patch

and note in the commit messages for the 1, 2... patches
that they're purely code movement with no textual changes.

thanks
-- PMM