On 11/12/2010 10:15 AM, Matthias Bolte wrote:
>>> HACKING | 602 +++++++++++++++++++++++++++++---------------------
>>
>> A lot of this diff is whitespace; the rest looks like it is pulling in
>> missed bits. Overall, it looks pretty decent; and it's certainly more
>> maintainable!
>>
Patch 1 review:
> Tweak pre tags to achieve proper indentation of their
> plaintext representation. Also use more b/i/code tags.
> ---
> docs/hacking.html.in | 351 +++++++++++++++++++++++++++-----------------------
> 1 files changed, 189 insertions(+), 162 deletions(-)
>
> @@ -336,57 +358,57 @@
> </p>
>
> <ul>
> - <li><p>eg to allocate a single object:</p>
> -
> + <li><p>e.g. to allocate a single object:</p>
Using "e.g." in every <li> of this bullet-list seems redundant; but
cleaning that up can be a separate patch (in fact, I've already written
the patch, as part of the VIR_RESIZE series that I'm trying to revive,
but haven't pushed it to the list yet until yours is in, so I'll get
more merge conflicts if you change wording now :)
> <li><p>eg close a file descriptor in an error path, without losing
> the previous errno value</p>
Should errno be surrounded in <code>, since it is a variable name?
> +<pre>
> char *
> - somefunction(...) {
> + somefunction(...)
> + {
I like this change...
> @@ -569,13 +595,13 @@
> #include "util.h" Any libvirt internal header files.
> #include "buf.h"
>
> - static myInternalFunc () The actual code.
> + static myInternalFunc() The actual code.
> {
and this is already wrong, since it lacks a return type. Let's adjust
it to match the same style:
static int
myInternalFunc()
{
there are several code analysis tools that work better if all function
implementation names start on column 0, with return type and other
decorations on the previous line. Even though we don't yet have a
consistent style throughout libvirt, we might as well have a single
style in the hacking guide.
ACK, either as-is, or if you want to make a few tweaks per my above
comments. Either way, it's a good cleanup and documentation-only, so I
don't need to see a v2 before you push this patch.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list