| Message ID | 1594140062-23522-1-git-send-email-aleksandar.qemu.devel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Patchew URL: https://patchew.org/QEMU/1594140062-23522-1-git-send-email-aleksandar.qemu.devel@gmail.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 1594140062-23522-1-git-send-email-aleksandar.qemu.devel@gmail.com
Subject: [PULL 0/3] MIPS + TCG Continuous Benchmarking queue for July 7th, 2020
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
710fb08..c8eaf81 master -> master
- [tag update] patchew/20200701183949.398134-1-atish.patra@wdc.com -> patchew/20200701183949.398134-1-atish.patra@wdc.com
* [new tag] patchew/20200707180836.5435-1-vr_qemu@t-online.de -> patchew/20200707180836.5435-1-vr_qemu@t-online.de
Switched to a new branch 'test'
962f29e scripts/performance: Add dissect.py script
e646d73 disas: mips: Add Loongson 2F disassembler
99e009f target/mips: fpu: Fix recent regression in add.s after 1ace099f2a
=== OUTPUT BEGIN ===
1/3 Checking commit 99e009f3955b (target/mips: fpu: Fix recent regression in add.s after 1ace099f2a)
2/3 Checking commit e646d73f8b6e (disas: mips: Add Loongson 2F disassembler)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#60:
new file mode 100644
ERROR: space prohibited between function name and open parenthesis '('
#10795: FILE: include/disas/dis-asm.h:399:
+int print_insn_loongson2f (bfd_vma, disassemble_info*);
total: 1 errors, 1 warnings, 10763 lines checked
Patch 2/3 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/3 Checking commit 962f29e09a89 (scripts/performance: Add dissect.py script)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#33:
new file mode 100755
total: 0 errors, 1 warnings, 165 lines checked
Patch 3/3 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/1594140062-23522-1-git-send-email-aleksandar.qemu.devel@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On 07/07/2020 18.40, Aleksandar Markovic wrote: > The following changes since commit 710fb08fd297d7a92163debce1959fae8f3b6ed7: > > Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2020-07-06' into staging (2020-07-07 12:41:15 +0100) > > are available in the git repository at: > > https://github.com/AMarkovic/qemu tags/mips-queue-jul-07-2020 > > for you to fetch changes up to fa6e7da119b6da4067e757924e165bc737bb1260: > > scripts/performance: Add dissect.py script (2020-07-07 18:32:20 +0200) > > ---------------------------------------------------------------- > > MIPS + TCG Continuous Benchmarking queue for July 7th, 2020 > > Highlights: > > - Fix for a regression in FPU emulation add.s. > - Add Loongson 2F disassembler. > - Add a script for a GSoC project. > > Note: > > - A checkpatch error and a checkpatch warning are known and > should be ignored. > > ---------------------------------------------------------------- > > Ahmed Karaman (1): > scripts/performance: Add dissect.py script > > Alex Richardson (1): > target/mips: fpu: Fix recent regression in add.s after 1ace099f2a > > Stefan Brankovic (1): > disas: mips: Add Loongson 2F disassembler > > configure | 1 + > disas/loongson2f.h | 2562 +++++++++++++ > include/disas/dis-asm.h | 1 + > include/exec/poison.h | 1 + > target/mips/cpu.c | 6 + > target/mips/fpu_helper.c | 2 +- > MAINTAINERS | 1 + > disas/Makefile.objs | 1 + > disas/loongson2f.cpp | 8154 ++++++++++++++++++++++++++++++++++++++++ Honestly, no. Peter, please don't merge this pull request. That disassembler source code is really huge, and I think someone should give this a *proper* review first before we include this in our repo. I just had a quick look at it, and I don't think that it is in the right shape already. For example, there are hard-coded magic numbers there, like: bool ADD::disas_output(disassemble_info *info) +{ + char alias1[5]; + char alias2[5]; + char alias3[5]; ... and in a completely different function, this hard-coded 5 is used again: +void Instruction32::getAlias(char *buffer, int regNo) +{ + switch (regNo) { + case 0: + strncpy(buffer, "zero", 5); + break; + case 1: + strncpy(buffer, "at", 5); ... That definitely needs to be turned into a proper #define or the length needs to be passed as parameter to the function. Also the coding style is weird in a couple of places, and there were checkpatch warnings. Apart from that, Paolo asked you to take a break from MIPS maintainership for a while, Aleksandar. I strongly support that suggestion. Your derogatory behavior during the last weeks, especially in the last one, looked completely unacceptable to me. In my opinion you really need some time to reflect yourself. You, and we all as a community, now cannot continue just like nothing happened. Thanks, Thomas
I haven't looked at the disassembler code; assuming it comes from an upstream code base I don't think we should treat it differently from the ARM disassembler (or for that matter the binutils ones) and basically handle it as a black box for which we don't really care about the code quality or style. It's not security sensitive code, and forking the upstream is probably not justified. On the other hand the pull request should have explained the provenance of the code (even if only in the subject line of the relevant patch). Regarding the recommendation of a "vacation" after last week's facts, QEMU's enforcing of behavior standards so far has been informal, therefore my suggestion cannot be anything more than a suggestion, though a strongly recommended one. I can also understand the soft freeze pressure. I defer to Peter as to whether to merge this pull request or not due to Thomas's objections, but I do wish to reinforce that taking some time off to cool down can often times be a good idea. Thanks, Paolo Il mar 7 lug 2020, 22:20 Thomas Huth <thuth@redhat.com> ha scritto: > On 07/07/2020 18.40, Aleksandar Markovic wrote: > > The following changes since commit > 710fb08fd297d7a92163debce1959fae8f3b6ed7: > > > > Merge remote-tracking branch > 'remotes/huth-gitlab/tags/pull-request-2020-07-06' into staging (2020-07-07 > 12:41:15 +0100) > > > > are available in the git repository at: > > > > https://github.com/AMarkovic/qemu tags/mips-queue-jul-07-2020 > > > > for you to fetch changes up to fa6e7da119b6da4067e757924e165bc737bb1260: > > > > scripts/performance: Add dissect.py script (2020-07-07 18:32:20 +0200) > > > > ---------------------------------------------------------------- > > > > MIPS + TCG Continuous Benchmarking queue for July 7th, 2020 > > > > Highlights: > > > > - Fix for a regression in FPU emulation add.s. > > - Add Loongson 2F disassembler. > > - Add a script for a GSoC project. > > > > Note: > > > > - A checkpatch error and a checkpatch warning are known and > > should be ignored. > > > > ---------------------------------------------------------------- > > > > Ahmed Karaman (1): > > scripts/performance: Add dissect.py script > > > > Alex Richardson (1): > > target/mips: fpu: Fix recent regression in add.s after 1ace099f2a > > > > Stefan Brankovic (1): > > disas: mips: Add Loongson 2F disassembler > > > > configure | 1 + > > disas/loongson2f.h | 2562 +++++++++++++ > > include/disas/dis-asm.h | 1 + > > include/exec/poison.h | 1 + > > target/mips/cpu.c | 6 + > > target/mips/fpu_helper.c | 2 +- > > MAINTAINERS | 1 + > > disas/Makefile.objs | 1 + > > disas/loongson2f.cpp | 8154 > ++++++++++++++++++++++++++++++++++++++++ > > Honestly, no. Peter, please don't merge this pull request. > > That disassembler source code is really huge, and I think someone should > give this a *proper* review first before we include this in our repo. I > just had a quick look at it, and I don't think that it is in the right > shape already. For example, there are hard-coded magic numbers there, like: > > bool ADD::disas_output(disassemble_info *info) > +{ > + char alias1[5]; > + char alias2[5]; > + char alias3[5]; > ... > > and in a completely different function, this hard-coded 5 is used again: > > +void Instruction32::getAlias(char *buffer, int regNo) > +{ > + switch (regNo) { > + case 0: > + strncpy(buffer, "zero", 5); > + break; > + case 1: > + strncpy(buffer, "at", 5); > ... > > That definitely needs to be turned into a proper #define or the length > needs to be passed as parameter to the function. > > Also the coding style is weird in a couple of places, and there were > checkpatch warnings. > > Apart from that, Paolo asked you to take a break from MIPS > maintainership for a while, Aleksandar. I strongly support that > suggestion. Your derogatory behavior during the last weeks, especially > in the last one, looked completely unacceptable to me. In my opinion you > really need some time to reflect yourself. You, and we all as a > community, now cannot continue just like nothing happened. > > Thanks, > Thomas > >
The following changes since commit 710fb08fd297d7a92163debce1959fae8f3b6ed7: Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2020-07-06' into staging (2020-07-07 12:41:15 +0100) are available in the git repository at: https://github.com/AMarkovic/qemu tags/mips-queue-jul-07-2020 for you to fetch changes up to fa6e7da119b6da4067e757924e165bc737bb1260: scripts/performance: Add dissect.py script (2020-07-07 18:32:20 +0200) ---------------------------------------------------------------- MIPS + TCG Continuous Benchmarking queue for July 7th, 2020 Highlights: - Fix for a regression in FPU emulation add.s. - Add Loongson 2F disassembler. - Add a script for a GSoC project. Note: - A checkpatch error and a checkpatch warning are known and should be ignored. ---------------------------------------------------------------- Ahmed Karaman (1): scripts/performance: Add dissect.py script Alex Richardson (1): target/mips: fpu: Fix recent regression in add.s after 1ace099f2a Stefan Brankovic (1): disas: mips: Add Loongson 2F disassembler configure | 1 + disas/loongson2f.h | 2562 +++++++++++++ include/disas/dis-asm.h | 1 + include/exec/poison.h | 1 + target/mips/cpu.c | 6 + target/mips/fpu_helper.c | 2 +- MAINTAINERS | 1 + disas/Makefile.objs | 1 + disas/loongson2f.cpp | 8154 ++++++++++++++++++++++++++++++++++++++++ scripts/performance/dissect.py | 165 + 10 files changed, 10893 insertions(+), 1 deletion(-) create mode 100644 disas/loongson2f.h create mode 100644 disas/loongson2f.cpp create mode 100755 scripts/performance/dissect.py