[Qemu-devel] Why do we typedef every struct on QEMU?

[Qemu-devel] Why do we typedef every struct on QEMU?

[Eduardo Habkost]

I have been looking at patches that touch typedefs.h and
wondering: why do we make typedefs.h necessary at all?  Why do we
always add typedefs for every struct and union type in QEMU?

Why do we prefer to write this:

----- qemu/typedefs.h:
typedef struct SomeType SomeType;
----------------------

----- qemu/somecode.h:
#include <qemu/typedefs.h>

int some_function(SomeType *a);
----------------------


...instead of simply writing this:?

----- qemu/somecode.h:
struct SomeType;
int some_function(struct SomeType *a);
----------------------

Is the maintenance burden of typedefs.h worth it?

-- 
Eduardo

Re: [Qemu-devel] Why do we typedef every struct on QEMU?

[Peter Maydell]

On 17 July 2018 at 20:50, Eduardo Habkost <ehabkost@redhat.com> wrote:
> I have been looking at patches that touch typedefs.h and
> wondering: why do we make typedefs.h necessary at all?  Why do we
> always add typedefs for every struct and union type in QEMU?
>
> Why do we prefer to write this:
>
> ----- qemu/typedefs.h:
> typedef struct SomeType SomeType;
> ----------------------
>
> ----- qemu/somecode.h:
> #include <qemu/typedefs.h>
>
> int some_function(SomeType *a);
> ----------------------
>
>
> ...instead of simply writing this:?
>
> ----- qemu/somecode.h:
> struct SomeType;
> int some_function(struct SomeType *a);
> ----------------------
>
> Is the maintenance burden of typedefs.h worth it?

Personally I don't like typing "struct " all the time
when I'm using the type...

Note also that most typedefed structs don't go in
typedefs.h -- the typedef is defined with the struct.
A quick rough count suggests less than 10% are in
typedefs.h. We only put in the ones where there's
a lot of code that wants to use pointers to them
as opaque types and isn't pulling in the header where
the full struct is defined.

Also, this is one of the few bits of QEMU coding style
where we're pretty consistent, so I'd rather not
let it open to more free variation.

thanks
-- PMM

Re: [Qemu-devel] Why do we typedef every struct on QEMU?

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 17 July 2018 at 20:50, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> I have been looking at patches that touch typedefs.h and
>> wondering: why do we make typedefs.h necessary at all?  Why do we
>> always add typedefs for every struct and union type in QEMU?
>>
>> Why do we prefer to write this:
>>
>> ----- qemu/typedefs.h:
>> typedef struct SomeType SomeType;
>> ----------------------
>>
>> ----- qemu/somecode.h:
>> #include <qemu/typedefs.h>
>>
>> int some_function(SomeType *a);
>> ----------------------
>>
>>
>> ...instead of simply writing this:?
>>
>> ----- qemu/somecode.h:
>> struct SomeType;
>> int some_function(struct SomeType *a);
>> ----------------------
>>
>> Is the maintenance burden of typedefs.h worth it?
>
> Personally I don't like typing "struct " all the time
> when I'm using the type...

For a different point of view (which I happen to share), see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=024ddc0ce1049298bd3cae60ae45d9c5f0fb8b9c#n318

> Note also that most typedefed structs don't go in
> typedefs.h -- the typedef is defined with the struct.
> A quick rough count suggests less than 10% are in
> typedefs.h. We only put in the ones where there's
> a lot of code that wants to use pointers to them
> as opaque types and isn't pulling in the header where
> the full struct is defined.

Correct.

> Also, this is one of the few bits of QEMU coding style
> where we're pretty consistent, so I'd rather not
> let it open to more free variation.

Agree on the value of consistency.

In my not particularly humble opinion, the QEMU coding style is one big
mistake.

This is the number of C source files and lines back when CODING_STYLE
was created[*]:

    $ git-checkout e68b98dc723
    [...]
    $ git-ls-tree -r HEAD --name-only | grep -c '\.[ch]$'
    786
    $ git-ls-tree -r HEAD --name-only | grep '\.[ch]$' | xargs wc -l | tail -n 1
      480493 total

They've since grown more than five-fold and three-fold, respectively:

    $ git-checkout v3.0.0-rc1
    [...]
    $ git-ls-tree -r HEAD --name-only | grep -c '\.[ch]$'
    4021
    $ git-ls-tree -r HEAD --name-only | grep '\.[ch]$' | xargs wc -l | tail -n 1
     1653607 total

Let's count added and deleted lines in the diff between the two
versions, ignoring added files:

    $ git-diff --numstat --diff-filter=a -l7000 e68b98dc723 | awk '/\.[ch]}?$/ { a+=$1; d+=$2 } END { print a, d }'
    86260 301104

Of the 480k lines we had back then, 300k are gone.

Thus, roughly 180k of 1654k lines still go back to e68b98dc723.  That's
less than 11%.

Had we switched to an established style (such as the kernel's) back
then, the one time pain of reformatting and its impact on git-blame
would've been long forgotten.

Instead, we've repeatedly wasted time on debating which kind of ugly we
hate less, and all we can show for our troubles is CODING_STYLE.  Which
leaves a whole lot more questions open than it answers, so we can keep
enjoying style debates.

That the code shows anything resembling consistency at all is a
testament to humanity's yearning for order within a chaotic world.


[*] "With the help of some Limoncino".

Re: [Qemu-devel] Why do we typedef every struct on QEMU?

[Thomas Huth]

On 19.07.2018 08:42, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On 17 July 2018 at 20:50, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> I have been looking at patches that touch typedefs.h and
>>> wondering: why do we make typedefs.h necessary at all?  Why do we
>>> always add typedefs for every struct and union type in QEMU?
>>>
>>> Why do we prefer to write this:
>>>
>>> ----- qemu/typedefs.h:
>>> typedef struct SomeType SomeType;
>>> ----------------------
>>>
>>> ----- qemu/somecode.h:
>>> #include <qemu/typedefs.h>
>>>
>>> int some_function(SomeType *a);
>>> ----------------------
>>>
>>>
>>> ...instead of simply writing this:?
>>>
>>> ----- qemu/somecode.h:
>>> struct SomeType;
>>> int some_function(struct SomeType *a);
>>> ----------------------
>>>
>>> Is the maintenance burden of typedefs.h worth it?
>>
>> Personally I don't like typing "struct " all the time
>> when I'm using the type...
> 
> For a different point of view (which I happen to share), see
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=024ddc0ce1049298bd3cae60ae45d9c5f0fb8b9c#n318

FWIW, as somebody who also has to write some kernel code occasionally,
I'd also prefer if we could get closer to the kernel coding style here
again.

That would also avoid problems like this that we used to hit regularly:

 https://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg02695.html

(well, that problem will likely go away automatically since we now don't
support these distros anymore, but still...)

 Thomas

Re: [Qemu-devel] Why do we typedef every struct on QEMU?

[Paolo Bonzini]

On 19/07/2018 08:42, Markus Armbruster wrote:
> For a different point of view (which I happen to share), see
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=024ddc0ce1049298bd3cae60ae45d9c5f0fb8b9c#n318

I think that does not entirely apply to us:

> It's a **mistake** to use typedef for structures and pointers. When you see a
> 
> .. code-block:: c
> 
> 
> 	vps_t a;
> 
> in the source, what does it mean?
> In contrast, if it says
> 
> .. code-block:: c
> 
> 	struct virtual_container *a;
> 
> you can actually tell what ``a`` is.

We can actually tell that "a" is a struct in QEMU, because that's the
only place where we use CamelCase identifiers (typedefs for integer
types use lowercase names).  I agree with Linus that it's bad to use
typedef for pointers.

Thanks,

Paolo

Re: [Qemu-devel] Why do we typedef every struct on QEMU?

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Thu, Jul 19, 2018 at 08:42:05AM +0200, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On 17 July 2018 at 20:50, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Instead, we've repeatedly wasted time on debating which kind of ugly we
> hate less, and all we can show for our troubles is CODING_STYLE.  Which
> leaves a whole lot more questions open than it answers, so we can keep
> enjoying style debates.
> 
> That the code shows anything resembling consistency at all is a
> testament to humanity's yearning for order within a chaotic world.

Going back to something concrete after this nice philosophical musing:

The coding style checker (checkpatch.pl) seems like a huge success to
me.  Without it, achieving consistency is futile.

checkpatch.pl defines the true coding style of QEMU - the subset that
can be automatically checked by patchew.  If we want to follow a coding
style, implementing the rules in checkpatch.pl is important.

(Though I'll be the first to admit that checkpatch.pl has many
limitations.)

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] Why do we typedef every struct on QEMU?

[Daniel P. Berrangé]

On Fri, Jul 27, 2018 at 02:03:17PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jul 19, 2018 at 08:42:05AM +0200, Markus Armbruster wrote:
> > Peter Maydell <peter.maydell@linaro.org> writes:
> > 
> > > On 17 July 2018 at 20:50, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > Instead, we've repeatedly wasted time on debating which kind of ugly we
> > hate less, and all we can show for our troubles is CODING_STYLE.  Which
> > leaves a whole lot more questions open than it answers, so we can keep
> > enjoying style debates.
> > 
> > That the code shows anything resembling consistency at all is a
> > testament to humanity's yearning for order within a chaotic world.
> 
> Going back to something concrete after this nice philosophical musing:
> 
> The coding style checker (checkpatch.pl) seems like a huge success to
> me.  Without it, achieving consistency is futile.
> 
> checkpatch.pl defines the true coding style of QEMU - the subset that
> can be automatically checked by patchew.  If we want to follow a coding
> style, implementing the rules in checkpatch.pl is important.

Sadly that is not enough and arguably making life worse in some ways.

The core problem is that we never change any existing code to follow
the coding style rules we add. We only require new code to use it. We
have plenty of code that goes years between being touched by a patch,
so we've got countless different coding styles present across the
source tree. What's worse is that when submitting new patches, you have
to merge unrelated style cleanup changes into your functional changes,
just so the patch can pass checkpatch.pl, or go back & clean up enough
of the existing code in a separate patch, so that your patch doesn't
trigger a checkpatch.pl violation :-(

I wish we'd do a blanket cleanup to get the entire codebase passing
checkpatch. Despite the downside that it'd create a conflict point
in the history for people cherry-picking to older branches in the short
term, I think it would be a net win over the long term (3+ years) to
actually have a real coding style that is consistently followed.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] Why do we typedef every struct on QEMU?

[Peter Maydell]

On 27 July 2018 at 14:03, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Jul 19, 2018 at 08:42:05AM +0200, Markus Armbruster wrote:
>> That the code shows anything resembling consistency at all is a
>> testament to humanity's yearning for order within a chaotic world.
>
> Going back to something concrete after this nice philosophical musing:
>
> The coding style checker (checkpatch.pl) seems like a huge success to
> me.  Without it, achieving consistency is futile.
>
> checkpatch.pl defines the true coding style of QEMU - the subset that
> can be automatically checked by patchew.  If we want to follow a coding
> style, implementing the rules in checkpatch.pl is important.

I think that overall if you look at QEMU's code we have a lot of
consistency in that we follow the common "open source Unix C code"
coding style. For instance if you look at:

static BOOL GetSystemName(
    const void *pvSystemStore,
    DWORD dwFlags,
    PENUM_ARG pEnumArg,
    LPCWSTR *ppwszSystemName ) {

you probably recognize immediately that it's Windows style,
and we don't require checkpatch to avoid code that is that
sharply divergent from the cultural norm.

CODING_STYLE has a lot of nitpicky stuff because we mostly
disagree about the edge cases rather than the core things, I think.

thanks
-- PMM