[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] clang-tidy: use g_new() family of functi
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] clang-tidy: use g_new() family of functions |
Date: |
Tue, 05 Sep 2017 17:33:42 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> Hi
>
> On Tue, Sep 5, 2017 at 2:56 PM Markus Armbruster <address@hidden> wrote:
>
>> Marc-André Lureau <address@hidden> writes:
>>
>> > Hi,
>> >
>> > I have a series of changes generated with clang-tidy qemu [1] pending
>> > for review [2].
>> >
>> > It translates calloc/*malloc*/*realloc() calls to
>> > g_new/g_newa/g_new0/g_renew() where the argument is a sizeof(T) [* N].
>>
>> Only for *type* T, or for arbitrary T?
>>
>>
> If sizeof() argument is a type:
> https://github.com/elmarco/clang-tools-extra/blob/master/clang-tidy/qemu/UseGnewCheck.cpp#L65
>
> For type T, we've done the transformation repeatedly with Coccinelle,
>> first in commit b45c03f (commit message appended). Repeatedly, because
>> they creep back in.
>>
>> How does your technique compare to this prior one?
>>
>
> Well, for one, I have a lot of trouble running coccinelle, mostly because
> it is slow and/or eat all my memory.
It's a bit of a pig, but it doesn't get anywhere close to prohibitive
even on my rather modest development box. Example run with a minor
variation of the semantic patch from commit b45c03f:
$ time spatch --in-place --sp-file g_new.cocci --macro-file
scripts/cocci-macro-file.h --use-idutils --dir .
[...]
real 0m42.379s
user 0m38.988s
sys 0m2.179s
$ git-diff --shortstat
123 files changed, 211 insertions(+), 216 deletions(-)
Note that "--use-idutils --dir ." directs spatch to work only on files
that need work. There's also --use-glimpse. You can easily do the
directing by hand with a judicious `git-grep ...`. Your experience can
be far worse if you feed it source files indiscriminately.
> Afaik, coccinelle also has trouble
> parsing the code in various cases.
Yes, but anything else that doesn't is almost certainly not tackling the
full problem.
Preprocessing C is easy enough. Parsing preprocessed C is easy enough.
Parsing unpreprocessed C into something that's suitable for transforming
is darn hard.
Doesn't mean that clang-tidy couldn't be doing a better job for our
code.
> I have also trouble writing coccinelle
> semantic patch...
Point taken.
> And, I gave up.
I've found it frustrating at times, but overall too useful to pass up.
I'd love to buy a well-written book on the thing.
> clang-tidy in comparison, is quite fast (it takes a minute to apply on qemu
Same order of magnitude as my spatch run above.
> code base). I find it easier to write a tidy check, and more friendly
> command line / output etc:
>
> /tmp/test.c:3:17: warning: use g_new() instead [qemu-use-gnew]
> int *f = (int*)malloc(sizeof(int) * 2);
> ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
> g_new(int, 2)
>
> clang-tidy should also respect clang-format rules when modifying the code
> (see also
> https://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg05762.html).
>
> I also imagine it should be possible to integrate to clang warnings in the
> future (similar to libreoffice clang plugins for ex). All in all,
> coccinelle or clang-tidy are probably both capable of doing this job, but
> clang-tidy is snappier and has more potentials for toolchain integration.
Suggest you show us cool things you can do with clang-tidy that haven't
been done with Coccinelle :)
[...]