[Libvir] RFC: Add a short doc with libvirt coding style guidelines
There's a few coding style guidelines we've kind of informally agreed upon wrt to libvirt patches, and perhaps more that we ought to make decisions on. For the benefit of people sending patches I reckon its worth writing these down. So I'm attaching a proposed doc 'HACKING' in which we can detail the guidelines. And yes, we don't uniformly comply with these guidelines in all places....all the more reason to list them & fix cases where we don't comply :-) Thoughts.. ? Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Hey, There's a few more obvious things missing, I think - e.g. should new code use lower_case_with_underscores naming style or mixedCase ? On Sun, 2008-01-13 at 17:39 +0000, Daniel P. Berrange wrote:
2. Do not use conditionals in front of 'free'.
eg Instead of
if (foo) free(foo)
Use
free(foo)
That's only safe with glibc, right ? Cheers, Mark.
On Mon, Jan 14, 2008 at 08:40:35AM +0000, Mark McLoughlin wrote:
Hey, There's a few more obvious things missing, I think - e.g. should new code use lower_case_with_underscores naming style or mixedCase ?
Really ? About 70% of our code currently uses mixed case, no underscores. The QEMU/network driver file & mdns file are the main ones which don't.
On Sun, 2008-01-13 at 17:39 +0000, Daniel P. Berrange wrote:
2. Do not use conditionals in front of 'free'.
eg Instead of
if (foo) free(foo)
Use
free(foo)
That's only safe with glibc, right ?
The man page says its C89 standard and the nice folks at Wine have done some background checks which show its safe on any modern UNIX... http://www.winehq.org/pipermail/wine-patches/2006-October/031544.html Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
On Mon, 2008-01-14 at 14:11 +0000, Daniel P. Berrange wrote:
On Mon, Jan 14, 2008 at 08:40:35AM +0000, Mark McLoughlin wrote:
Hey, There's a few more obvious things missing, I think - e.g. should new code use lower_case_with_underscores naming style or mixedCase ?
Really ? About 70% of our code currently uses mixed case, no underscores. The QEMU/network driver file & mdns file are the main ones which don't.
But does the 70/30 split imply that it's okay for people writing new code for libvirt to use lower_case_with_underscores or not? I'm guessing not, especially since all your storage stuff is mixedCase, but it'd be worth pointing out in HACKING. My main point was that some details on what is considered to be the libvirt coding style (aside form indentation) might help people.
On Sun, 2008-01-13 at 17:39 +0000, Daniel P. Berrange wrote:
2. Do not use conditionals in front of 'free'.
eg Instead of
if (foo) free(foo)
Use
free(foo)
That's only safe with glibc, right ?
The man page says its C89 standard and the nice folks at Wine have done some background checks which show its safe on any modern UNIX...
http://www.winehq.org/pipermail/wine-patches/2006-October/031544.html
Cool stuff, new to me. Cheers, Mark.
On Mon, Jan 14, 2008 at 02:35:10PM +0000, Mark McLoughlin wrote:
On Mon, 2008-01-14 at 14:11 +0000, Daniel P. Berrange wrote:
On Mon, Jan 14, 2008 at 08:40:35AM +0000, Mark McLoughlin wrote:
Hey, There's a few more obvious things missing, I think - e.g. should new code use lower_case_with_underscores naming style or mixedCase ?
Really ? About 70% of our code currently uses mixed case, no underscores. The QEMU/network driver file & mdns file are the main ones which don't.
But does the 70/30 split imply that it's okay for people writing new code for libvirt to use lower_case_with_underscores or not? I'm guessing not, especially since all your storage stuff is mixedCase, but it'd be worth pointing out in HACKING.
Yeah we've never really discussed it at all before. I decided to do the storage stuff as mixedCase, because I want to minimise changes in case we decide to want to expose the 'struct virStorageDef' as public API to let people feed in a formal data structure instead of XML. At this point I think it may well be desirable to make virStorageDef a public API. Once I've got NPIV impl done we'll have covered all the different storage areas I think we should have good confidence that the struct is stable enough to consider making public for convenience of people creating volumes/pools.
My main point was that some details on what is considered to be the libvirt coding style (aside form indentation) might help people.
Yes, definitely a good thing to make a note of. Dan -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
Thoughts.. ?
Thanks for writing that up. I like them all -- of course :-)
Libvirt Hacking Guidelines -------------------------- 1. Apply sizeof to the return variable, not the type. 2. Do not use conditionals in front of 'free'. 3. Include "config.h" in all source files 4. Indent in 4 space units, without tabs 5. Ensure any user visible strings are marked for translation
Here are a few proposed additions: (IMHO, each that we agree on should be automatically checked) * avoid global-scoped variables * avoid trailing blanks and blank lines at end of file * avoid SPACE-TAB (i.e., " ") esp. in indentation, but even in other contexts, like globs and regular expressions: i.e., prefer to use the TAB-SPACE sequence, grep '[ ]$' ..., to the SPACE-TAB one. Of course, it's even better if you can use the syntax where the TAB is actually visible: /[\t ]$/. * use const-correct types (I don't know how to check this automatically) With these, it is easier (or "possible") to reject invalid inputs: * avoid atof, atoi, atol, etc. * if you use strto*, be very careful, or consider the xstrto* wrappers * avoid parsing with sscanf
Daniel P. Berrange wrote:
There's a few coding style guidelines we've kind of informally agreed upon wrt to libvirt patches, and perhaps more that we ought to make decisions on. For the benefit of people sending patches I reckon its worth writing these down. So I'm attaching a proposed doc 'HACKING' in which we can detail the guidelines. And yes, we don't uniformly comply with these guidelines in all places....all the more reason to list them & fix cases where we don't comply :-)
5. Ensure any user visible strings are marked for translation eg wrap all strings with _(...) virXendError(xend, VIR_ERR_INTERNAL_ERROR, _("domain information incomplete, missing domid")); Translating error messages is a bad idea: (1) Naive users ignore error messages anyway. (2) It makes the error message much less useful when doing an internet search for solutions. (3) If an error message is posted to this list, it's much less useful if it isn't in English. It seems a large body of our users are Japanese, so I'd like to hear what they think about this issue. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
"Richard W.M. Jones" <rjones@redhat.com> wrote:
Daniel P. Berrange wrote:
There's a few coding style guidelines we've kind of informally agreed upon wrt to libvirt patches, and perhaps more that we ought to make decisions on. For the benefit of people sending patches I reckon its worth writing these down. So I'm attaching a proposed doc 'HACKING' in which we can detail the guidelines. And yes, we don't uniformly comply with these guidelines in all places....all the more reason to list them & fix cases where we don't comply :-)
5. Ensure any user visible strings are marked for translation
eg wrap all strings with _(...)
virXendError(xend, VIR_ERR_INTERNAL_ERROR, _("domain information incomplete, missing domid"));
Translating error messages is a bad idea: (1) Naive users ignore error messages anyway. (2) It makes the error message much less useful when doing an internet search for solutions. (3) If an error message is posted to this list, it's much less useful if it isn't in English.
Hi Rich, I've always heard that support for translatable diagnostics is an important feature. While I'm no UI/internationalization expert, I would have thought that less-savvy users would need the messages more than others. If a user is taking the time to report a bug to an English-speaking list and to include a precise diagnostic, they're probably clueful enough to translate any such message for us. At worst, since we have the .po files, we can look up the translation manually -- or just use an online translator.
On Tue, Jan 15, 2008 at 03:49:32PM +0100, Jim Meyering wrote:
"Richard W.M. Jones" <rjones@redhat.com> wrote:
Daniel P. Berrange wrote:
There's a few coding style guidelines we've kind of informally agreed upon wrt to libvirt patches, and perhaps more that we ought to make decisions on. For the benefit of people sending patches I reckon its worth writing these down. So I'm attaching a proposed doc 'HACKING' in which we can detail the guidelines. And yes, we don't uniformly comply with these guidelines in all places....all the more reason to list them & fix cases where we don't comply :-)
5. Ensure any user visible strings are marked for translation
eg wrap all strings with _(...)
virXendError(xend, VIR_ERR_INTERNAL_ERROR, _("domain information incomplete, missing domid"));
Translating error messages is a bad idea: (1) Naive users ignore error messages anyway. (2) It makes the error message much less useful when doing an internet search for solutions. (3) If an error message is posted to this list, it's much less useful if it isn't in English.
Hi Rich,
I've always heard that support for translatable diagnostics is an important feature.
While I'm no UI/internationalization expert, I would have thought that less-savvy users would need the messages more than others.
If a user is taking the time to report a bug to an English-speaking list and to include a precise diagnostic, they're probably clueful enough to translate any such message for us. At worst, since we have the .po files, we can look up the translation manually -- or just use an online translator.
If you look at glibc - all the strerror() stuff is translated, and we feed that into our own error messages. So if we didn't translate our own, we'd be giving users error messages which contain a mix of englissh and translated stuff. Our users have been very proactive about providing translations for our existing error messages, which suggests the translations are indeed important to them. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
participants (4)
-
Daniel P. Berrange -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones