Re: [Qemu-devel] clang-tidy: use g_new() family of functions

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Tue, Sep 5, 2017 at 2:56 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Marc-André Lureau <marcandre.lureau@gmail.com> 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 :)

[...]