add scripts/indent.sh

[Qemu-devel] [PATCH] add scripts/indent.sh

Posted by Gerd Hoffmann 6 years, 9 months ago

Script to reformat sources in qemu style.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 scripts/indent.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100755 scripts/indent.sh

diff --git a/scripts/indent.sh b/scripts/indent.sh
new file mode 100755
index 0000000000..8f045ecb1d
--- /dev/null
+++ b/scripts/indent.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+#
+# indent wrapper script, with args to format
+# source code according to qemu coding style.
+#
+indent	--ignore-profile	\
+	--k-and-r-style		\
+	--line-length 80	\
+	--indent-level 4	\
+	--no-tabs		\
+	"$@"
-- 
2.9.3

Re: [Qemu-devel] [PATCH] add scripts/indent.sh

Posted by Marc-André Lureau 6 years, 9 months ago

Hi

On Mon, Jul 17, 2017 at 12:18 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Script to reformat sources in qemu style.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>

A quite powerful approach would be to use clang-format:
https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg01278.html

It comes with a bunch of editor integration and scripts. My config could
use a bit more tweaking though, I haven't been using it for a while, and
newer version have more options.


> ---
>  scripts/indent.sh | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>  create mode 100755 scripts/indent.sh
>
> diff --git a/scripts/indent.sh b/scripts/indent.sh
> new file mode 100755
> index 0000000000..8f045ecb1d
> --- /dev/null
> +++ b/scripts/indent.sh
> @@ -0,0 +1,11 @@
> +#!/bin/sh
> +#
> +# indent wrapper script, with args to format
> +# source code according to qemu coding style.
> +#
> +indent --ignore-profile        \
> +       --k-and-r-style         \
> +       --line-length 80        \
> +       --indent-level 4        \
> +       --no-tabs               \
> +       "$@"
> --
> 2.9.3
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH] add scripts/indent.sh

Posted by Gerd Hoffmann 6 years, 9 months ago

On Mon, 2017-07-17 at 10:36 +0000, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jul 17, 2017 at 12:18 PM Gerd Hoffmann <kraxel@redhat.com>
> wrote:
> > Script to reformat sources in qemu style.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> A quite powerful approach would be to use clang-format: https://lists
> .gnu.org/archive/html/qemu-devel/2015-10/msg01278.html 
> 
> It comes with a bunch of editor integration and scripts. My config
> could use a bit more tweaking though, I haven't been using it for a
> while, and newer version have more options.

Hmm, I want reformat all audio stuff to qemu code style some day, the
current situation where patches are either break qemu code style or
lead to mixed-style in the audio source files isn't very good.

I don't mind much which tool we use for that, I picked indent mostly
because I know that one.  If clang-format can do that too, fine with
me.  I'm missing documentation though, there is no man-page and there
isn't anything in /usr/share/doc/clang-3.4.2 either ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] add scripts/indent.sh

Posted by Marc-André Lureau 6 years, 9 months ago

Hi

On Mon, Jul 17, 2017 at 12:49 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mon, 2017-07-17 at 10:36 +0000, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, Jul 17, 2017 at 12:18 PM Gerd Hoffmann <kraxel@redhat.com>
> > wrote:
> > > Script to reformat sources in qemu style.
> > >
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >
> > A quite powerful approach would be to use clang-format: https://lists
> > .gnu.org/archive/html/qemu-devel/2015-10/msg01278.html
> >
> > It comes with a bunch of editor integration and scripts. My config
> > could use a bit more tweaking though, I haven't been using it for a
> > while, and newer version have more options.
>
> Hmm, I want reformat all audio stuff to qemu code style some day, the
> current situation where patches are either break qemu code style or
> lead to mixed-style in the audio source files isn't very good.
>
> I don't mind much which tool we use for that, I picked indent mostly
> because I know that one.  If clang-format can do that too, fine with
> me.  I'm missing documentation though, there is no man-page and there
> isn't anything in /usr/share/doc/clang-3.4.2 either ...
>

Afaik, most of clang-tools documentation is only in html. The most recent
version is online at https://clang.llvm.org/docs/ClangFormat.html &
https://clang.llvm.org/docs/ClangFormatStyleOptions.html

Another advantage of using clang-format, is that it should be used by other
clang refactoring tools (as in my clang-tidy series for ex).


> cheers,
>   Gerd
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH] add scripts/indent.sh

Posted by Gerd Hoffmann 6 years, 9 months ago

On Mon, 2017-07-17 at 10:58 +0000, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jul 17, 2017 at 12:49 PM Gerd Hoffmann <kraxel@redhat.com>
> wrote:
> > On Mon, 2017-07-17 at 10:36 +0000, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Mon, Jul 17, 2017 at 12:18 PM Gerd Hoffmann <kraxel@redhat.com
> > >
> > > wrote:
> > > > Script to reformat sources in qemu style.
> > > >
> > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > >
> > > A quite powerful approach would be to use clang-format: https://l
> > ists
> > > .gnu.org/archive/html/qemu-devel/2015-10/msg01278.html 
> > >
> > > It comes with a bunch of editor integration and scripts. My
> > config
> > > could use a bit more tweaking though, I haven't been using it for
> > a
> > > while, and newer version have more options.
> > 
> > Hmm, I want reformat all audio stuff to qemu code style some day,
> > the
> > current situation where patches are either break qemu code style or
> > lead to mixed-style in the audio source files isn't very good.
> > 
> > I don't mind much which tool we use for that, I picked indent
> > mostly
> > because I know that one.  If clang-format can do that too, fine
> > with
> > me.  I'm missing documentation though, there is no man-page and
> > there
> > isn't anything in /usr/share/doc/clang-3.4.2 either ...
> 
> Afaik, most of clang-tools documentation is only in html. The most
> recent version is online at
> https://clang.llvm.org/docs/ClangFormat.html &
> https://clang.llvm.org/docs/ClangFormatStyleOptions.html

Can you add those links as comments to the .clang-format file?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] add scripts/indent.sh

Posted by Marc-André Lureau 6 years, 7 months ago

Hi

On Mon, Jul 17, 2017 at 1:14 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Mon, 2017-07-17 at 10:58 +0000, Marc-André Lureau wrote:
>> Hi
>>
>> On Mon, Jul 17, 2017 at 12:49 PM Gerd Hoffmann <kraxel@redhat.com>
>> wrote:
>> > On Mon, 2017-07-17 at 10:36 +0000, Marc-André Lureau wrote:
>> > > Hi
>> > >
>> > > On Mon, Jul 17, 2017 at 12:18 PM Gerd Hoffmann <kraxel@redhat.com
>> > >
>> > > wrote:
>> > > > Script to reformat sources in qemu style.
>> > > >
>> > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> > >
>> > > A quite powerful approach would be to use clang-format: https://l
>> > ists
>> > > .gnu.org/archive/html/qemu-devel/2015-10/msg01278.html
>> > >
>> > > It comes with a bunch of editor integration and scripts. My
>> > config
>> > > could use a bit more tweaking though, I haven't been using it for
>> > a
>> > > while, and newer version have more options.
>> >
>> > Hmm, I want reformat all audio stuff to qemu code style some day,
>> > the
>> > current situation where patches are either break qemu code style or
>> > lead to mixed-style in the audio source files isn't very good.
>> >
>> > I don't mind much which tool we use for that, I picked indent
>> > mostly
>> > because I know that one.  If clang-format can do that too, fine
>> > with
>> > me.  I'm missing documentation though, there is no man-page and
>> > there
>> > isn't anything in /usr/share/doc/clang-3.4.2 either ...
>>
>> Afaik, most of clang-tools documentation is only in html. The most
>> recent version is online at
>> https://clang.llvm.org/docs/ClangFormat.html &
>> https://clang.llvm.org/docs/ClangFormatStyleOptions.html
>
> Can you add those links as comments to the .clang-format file?

Sorry for the late reply, fwiw, my best attempt so far:
https://gist.github.com/elmarco/aa5e0b23567f46fb7f0e73cde586a0c1

It's not perfect, you can easily see the result by running inplace
with clang-format -i && git diff.

In particular:
- array declaration is often much uglier (with the indendation aligned
to open brace) - indent seems doing worse.
  also related: https://bugs.llvm.org/show_bug.cgi?id=18455
- it uses a space in "FOREACH (" macros - good/bad ?
- long line are splitted in less friendly way than manual split - also
true with indent
- qemu doesn't have BinPack true/false, it uses a mix - clang-format
should learn that - perhaps covered by
https://bugs.llvm.org/show_bug.cgi?id=31907
- we may want not to do #include ordering - I find it nicer though

The point is not to apply blindly clang-format over the whole code
base, but rather to give a decent default formatting when running
clang-tools, or simply when making a patch. For this, .clang-format is
necessary, and I would rather have it in qemu tree than in my own
working directory.



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] add scripts/indent.sh

Posted by Peter Maydell 6 years, 9 months ago

On 17 July 2017 at 11:17, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Script to reformat sources in qemu style.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  scripts/indent.sh | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>  create mode 100755 scripts/indent.sh
>
> diff --git a/scripts/indent.sh b/scripts/indent.sh
> new file mode 100755
> index 0000000000..8f045ecb1d
> --- /dev/null
> +++ b/scripts/indent.sh
> @@ -0,0 +1,11 @@
> +#!/bin/sh
> +#
> +# indent wrapper script, with args to format
> +# source code according to qemu coding style.
> +#
> +indent --ignore-profile        \
> +       --k-and-r-style         \
> +       --line-length 80        \
> +       --indent-level 4        \
> +       --no-tabs               \
> +       "$@"

A quick pass of this over target/arm suggests it's probably
missing some arguments. For instance, things it gets wrong:

(1) it's adding spaces after '*' in "SomeType *foo" parameters:

-static void aarch64_note_init(struct aarch64_note *note, DumpState *s,
+static void aarch64_note_init(struct aarch64_note *note, DumpState * s,

(2) weird extra space after cast:

-    ((TaskState *)cs->opaque)->swi_errno = err;
+    ((TaskState *) cs->opaque)->swi_errno = err;

(3) weird spacing around address-of operator:

-    cpu_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, (uint8_t *)&size, 4, 0);
+    cpu_memory_rw_debug(cs, arm_flen_buf(cpu) + 32, (uint8_t *) & size, 4, 0);

(4) weird spacing addition to a negative number constant:

-            return (uint32_t)-1;
+            return (uint32_t) - 1;

(5) mis-indentation of second line of multi-line assignment:

 static Property arm_cpu_reset_cbar_property =
-            DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0);
+DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0);

(6) incorrectly deletes spaces in a named-struct-field initializer:

-            .name = "IFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 1,
+            .name = "IFAR",.cp = 15,.crn = 6,.crm = 0,.opc1 = 0,.opc2 = 1,

(7) gets confused by HELPER() macro in function definition:

-int64_t HELPER(sdiv64)(int64_t num, int64_t den)
-{
+int64_t HELPER(sdiv64) (int64_t num, int64_t den) {

(8) It disagrees with our emacs indent config about level of
indent for expressions:

     return arm_feature(env, ARM_FEATURE_PMSA) &&
-           arm_feature(env, ARM_FEATURE_V7);
+        arm_feature(env, ARM_FEATURE_V7);

--no-space-after-casts ought to fix 2, but in practice doesn't
seem to...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] add scripts/indent.sh

Posted by no-reply@patchew.org 6 years, 9 months ago

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20170717101700.23552-1-kraxel@redhat.com
Subject: [Qemu-devel] [PATCH] add scripts/indent.sh

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1500279971-13875-1-git-send-email-peterx@redhat.com -> patchew/1500279971-13875-1-git-send-email-peterx@redhat.com
Switched to a new branch 'test'
8352810 add scripts/indent.sh

=== OUTPUT BEGIN ===
Checking PATCH 1/1: add scripts/indent.sh...
ERROR: code indent should never use tabs
#22: FILE: scripts/indent.sh:6:
+indent^I--ignore-profile^I\$

ERROR: code indent should never use tabs
#23: FILE: scripts/indent.sh:7:
+^I--k-and-r-style^I^I\$

ERROR: code indent should never use tabs
#24: FILE: scripts/indent.sh:8:
+^I--line-length 80^I\$

ERROR: code indent should never use tabs
#25: FILE: scripts/indent.sh:9:
+^I--indent-level 4^I\$

ERROR: code indent should never use tabs
#26: FILE: scripts/indent.sh:10:
+^I--no-tabs^I^I\$

ERROR: code indent should never use tabs
#27: FILE: scripts/indent.sh:11:
+^I"$@"$

total: 6 errors, 0 warnings, 11 lines checked

Your patch 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


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH] add scripts/indent.sh

Posted by Peter Maydell 6 years, 9 months ago

On 17 July 2017 at 19:11,  <no-reply@patchew.org> wrote:
> This series seems to have some coding style problems.

Very ironic :-)

thanks
-- PMM