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

Re: [libvirt] [PATCH v2 00/22] Remove unnecessary curly brackets around one-line bodies



On Thu, Nov 13, 2014 at 05:09:04PM -0700, Eric Blake wrote:
On 11/13/2014 04:52 PM, John Ferlan wrote:


On 11/13/2014 09:37 AM, Martin Kletzander wrote:
Brackets were removed by the following script in the root directory of
libvirt's repository:

  for i in $(git ls-files | grep '\.[ch]$'); do
    echo -n "$i ... ";
    emacs $i --batch --eval \
      '(replace-regexp "^\\([[:space:]]*\\(if\\|for\\|while\\).*)\\)[[:space:]]*{\\(\n[^\n;]*;[^\n;]*\\)\n[[:space:]]*}$" "\\1\\3" nil (point-min) (point-max))' \
    -f save-buffer &>/dev/null;
    echo done;
  done

v2:
 - Smaller patches, so there has to be none allowed through by
   moderators
 - Only curly brackets around one-line bodies with one-line preceding
   conditions removed
 - syntax-check! (with some other bracket-spacing.pl cleanups


ACK series in general - although something needs to be done about 17/22
and the comment in 22/22 probably needs an adjustment as well as a way
hopefully to indicate what rule is being violated.

I have not closely reviewed the series, and the perl in patch 22/22 is
complex enough that I don't feel I have enough time to do it justice in
a quick review today.  I'll trust John's review - he did have a good
point about multiline macro bodies needing do {} while (0) guards -
maybe we add yet another syntax check for that?  I'm guessing you wrote

I wouldn't believe we have multi-line macros without its own body.
Anyway, I fixed that.

I started working on the syntax-check for multiline macros without
do-while block, but it's going to be a bit painful.

the series by applying patch 22 first, then fixing in pieces until
things work; so if 22 is good and syntax-check passes, then all the
other patches prove we silenced the syntax problems (but not necessarily
that the transformations to silence the problems were correct, as was
the case with macros in tools/).


Actually, no.  I had few variations of that syntax-check, but the only
thing I was pretty sure about was that (regexp-replace) I did.  Then
it was just a matter of splitting it into small enough chunks so it
doesn't have to be allowed to list by moderator and so it makes sense.

At any rate, this series will be a conflict magnet to other pending
patches, so if we're going to take it, I don't want to hold it up for a
long time.  The fact that you have a syntax-check to enforce things is a
huge improvement over the v1 proposal.


I fixed everything and after discussion with John pushed the fixed
versions, thank you for spotting all the details.

Martin

Attachment: signature.asc
Description: Digital signature


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