Re: [PATCHv2 04/13] virbuf: add auto-indentation support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/19/2011 08:31 PM, Hai Dong Li wrote:
This email is just for your attention. I'm relatively new to work in a
community, so I didn't pay much attention to the readability of the
comments last email. It seems comments lie in a large patch like this is
easily to be omitted. So I cut the codes, leave codes associated with
the comments.

Yes, trimming to just relevant context is a must for any high-volume patch list. Also, separate your replies from the quoted material by blank lines, so it stands out better (visually, I find it easier to spot replies that appear in isolation, by scanning just the left column; not to mention that some mailers corrupt long lines on quoted replies where a long single-line paragraph in the original turns into a wrapped multi-line text with the first line quoted but subsequent lines unquoted; adding whitespace before your reply makes it obvious that you made the comment, rather than your mailer reformatting things).


+ virBufferAdjustIndent(buf, -2);
+ if (virBufferGetIndent(buf, false) != 1 ||
+ virBufferGetIndent(buf, true) != 1 ||
+ virBufferError(buf)) {
+ TEST_ERROR("Wrong indentation");
+ ret = -1;
+ }
So now buf->indent is 1. Go to the next step, the indent is given -2
again, see what will happen.
if virBufferAdjustIndent failed to check the indent overflow, the
buf->indent will be -1,too, so it may avoid the check
(virBufferGetIndent(buf, false) != -1) and (virBufferGetIndent(buf,
true) != -1).
+ virBufferAdjustIndent(buf, -2);
So I think -3 may be better.

Good idea; I've folded that into my patch.

--
Eric Blake   eblake@xxxxxxxxxx    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]