* [Qemu-devel] Why do we typedef every struct on QEMU? @ 2018-07-17 19:50 Eduardo Habkost 2018-07-17 20:06 ` Peter Maydell 0 siblings, 1 reply; 8+ messages in thread From: Eduardo Habkost @ 2018-07-17 19:50 UTC (permalink / raw) To: qemu-devel Cc: Philippe Mathieu-Daudé, Markus Armbruster, Dr. David Alan Gilbert, Paolo Bonzini, Peter Maydell 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Why do we typedef every struct on QEMU? 2018-07-17 19:50 [Qemu-devel] Why do we typedef every struct on QEMU? Eduardo Habkost @ 2018-07-17 20:06 ` Peter Maydell 2018-07-19 6:42 ` Markus Armbruster 0 siblings, 1 reply; 8+ messages in thread From: Peter Maydell @ 2018-07-17 20:06 UTC (permalink / raw) To: Eduardo Habkost Cc: QEMU Developers, Philippe Mathieu-Daudé, Markus Armbruster, Dr. David Alan Gilbert, Paolo Bonzini 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Why do we typedef every struct on QEMU? 2018-07-17 20:06 ` Peter Maydell @ 2018-07-19 6:42 ` Markus Armbruster 2018-07-19 8:18 ` Thomas Huth ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Markus Armbruster @ 2018-07-19 6:42 UTC (permalink / raw) To: Peter Maydell Cc: Eduardo Habkost, Paolo Bonzini, QEMU Developers, Dr. David Alan Gilbert, Philippe Mathieu-Daudé 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". ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Why do we typedef every struct on QEMU? 2018-07-19 6:42 ` Markus Armbruster @ 2018-07-19 8:18 ` Thomas Huth 2018-07-19 8:28 ` Paolo Bonzini 2018-07-27 13:03 ` Stefan Hajnoczi 2 siblings, 0 replies; 8+ messages in thread From: Thomas Huth @ 2018-07-19 8:18 UTC (permalink / raw) To: Markus Armbruster, Peter Maydell Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Eduardo Habkost, Dr. David Alan Gilbert, QEMU Developers 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Why do we typedef every struct on QEMU? 2018-07-19 6:42 ` Markus Armbruster 2018-07-19 8:18 ` Thomas Huth @ 2018-07-19 8:28 ` Paolo Bonzini 2018-07-27 13:03 ` Stefan Hajnoczi 2 siblings, 0 replies; 8+ messages in thread From: Paolo Bonzini @ 2018-07-19 8:28 UTC (permalink / raw) To: Markus Armbruster, Peter Maydell Cc: Eduardo Habkost, QEMU Developers, Dr. David Alan Gilbert, Philippe Mathieu-Daudé 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Why do we typedef every struct on QEMU? 2018-07-19 6:42 ` Markus Armbruster 2018-07-19 8:18 ` Thomas Huth 2018-07-19 8:28 ` Paolo Bonzini @ 2018-07-27 13:03 ` Stefan Hajnoczi 2018-07-27 13:14 ` Daniel P. Berrangé 2018-07-27 13:16 ` Peter Maydell 2 siblings, 2 replies; 8+ messages in thread From: Stefan Hajnoczi @ 2018-07-27 13:03 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Paolo Bonzini, Philippe Mathieu-Daudé, Eduardo Habkost, Dr. David Alan Gilbert, QEMU Developers [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Why do we typedef every struct on QEMU? 2018-07-27 13:03 ` Stefan Hajnoczi @ 2018-07-27 13:14 ` Daniel P. Berrangé 2018-07-27 13:16 ` Peter Maydell 1 sibling, 0 replies; 8+ messages in thread From: Daniel P. Berrangé @ 2018-07-27 13:14 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Markus Armbruster, Peter Maydell, Eduardo Habkost, QEMU Developers, Dr. David Alan Gilbert, Paolo Bonzini, Philippe Mathieu-Daudé 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 :| ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] Why do we typedef every struct on QEMU? 2018-07-27 13:03 ` Stefan Hajnoczi 2018-07-27 13:14 ` Daniel P. Berrangé @ 2018-07-27 13:16 ` Peter Maydell 1 sibling, 0 replies; 8+ messages in thread From: Peter Maydell @ 2018-07-27 13:16 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Markus Armbruster, Paolo Bonzini, Philippe Mathieu-Daudé, Eduardo Habkost, Dr. David Alan Gilbert, QEMU Developers 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-27 13:16 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-17 19:50 [Qemu-devel] Why do we typedef every struct on QEMU? Eduardo Habkost 2018-07-17 20:06 ` Peter Maydell 2018-07-19 6:42 ` Markus Armbruster 2018-07-19 8:18 ` Thomas Huth 2018-07-19 8:28 ` Paolo Bonzini 2018-07-27 13:03 ` Stefan Hajnoczi 2018-07-27 13:14 ` Daniel P. Berrangé 2018-07-27 13:16 ` Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.