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

Re: [Libvir] proposal: remove contradictory indentation directive



"Daniel P. Berrange" <berrange redhat com> wrote:
> On Wed, Apr 09, 2008 at 02:40:13PM +0100, Richard W.M. Jones wrote:
>> On Wed, Apr 09, 2008 at 02:43:22PM +0200, Jim Meyering wrote:
>> > Please don't add the "tab-width: 4" specifier.
>> > Specifying a tab-width at all in a new file with "indent-tabs-mode: nil"
>> > is a contradiction.  The latter says there should be no TABs, yet
>> > the former says "when there are, give them width 4."  Coding style
>> > guidelines are universal in their recommendations to stick with 8-byte
>> > TAB stops, independent of whether you actually use TAB or spaces.
>>
>> Agreed, good idea.
>
> ACK.
>
> Could we get a make syntax-check test to look for these bogus tabs too

I've done that.
Abbreviated patches below.

However, there are some "issues" to consider.
Any global space-changing delta like these is going to cause
trouble (conflicts) for people with pending changes and on branches.
This one isn't too bad (as these things go), since the TAB-to-space
change affects fewer than 1500 lines in 37 files.  However, the
new rule enforces the coding standard only in files with an existing
"indent-tabs-mode: nil" directive.  There are currently 42 .[ch] files
that don't have such a directive (excluding gnulib/).

    $ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \
      gnulib|wc -l
    42

If I were to do the same for those remaining files,
the TAB-to-space change would modify an additional 2817 lines
in 28 files:

    $ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \
    gnulib|xargs grep -E '^ *	'|wc -l
    2817
    $ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \
    gnulib|xargs grep -El '^ *	'|wc -l
    28

My opinion is that if it's worth doing the first, it's also worth
finishing the job, ... but then I don't have any huge re-architecting
changes in my queue.

However, I don't want to overstate the case either.
The cost of a few space-change-provoked merges is pretty low
in the grand scheme of things.

What say you?
Convert 'em all?

-----------------------------------------------------------------
Here are the interesting parts of the two changes.
First, remove the 'tab-width: 4' directives and make sure they
stay gone:

	remove all 'tab-width: 4' directives
	Makefile.maint (sc_prohibit_tab_width_4): New rule.
	Remove all of them, using this command:
	git grep -l 'tab-width: 4' \
	  | xargs perl -ni -e '/^\s*\*\s*tab-width:\s*4\s*$/ or print'

Signed-off-by: Jim Meyering <meyering redhat com>
---
 Makefile.maint                |    9 +++++++++
 proxy/libvirt_proxy.c         |    1 -
 ...
 tests/xmconfigtest.c          |    1 -
 78 files changed, 9 insertions(+), 77 deletions(-)

diff --git a/Makefile.maint b/Makefile.maint
index a357d74..fc21bf4 100644
--- a/Makefile.maint
+++ b/Makefile.maint
@@ -287,6 +287,15 @@ sc_trailing_blank:
 	  { echo '$(ME): found trailing blank(s)'			\
 		1>&2; exit 1; } || :

+sp=[	 ]
+# Some files used to containe a line like this: " * tab-width: 4",
+# contradicting an adjacent "indent-tabs-mode: nil" directive.
+# Make sure no more are introduced.
+sc_prohibit_tab_width_4:
+	@grep -E '^$(sp)*\*$(sp)*tab-width: 4$$' $$($(VC_LIST_EXCEPT)) && \
+	  { echo '$(ME): found bogus "tab-width:" directive(s)'		\
+		1>&2; exit 1; } || :
+
 # Match lines like the following, but where there is only one space
 # between the options and the description:
 #   -D, --all-repeated[=delimit-method]  print all duplicate lines\n
diff --git a/proxy/libvirt_proxy.c b/proxy/libvirt_proxy.c
index bed3ae8..b28a3bc 100644
--- a/proxy/libvirt_proxy.c
+++ b/proxy/libvirt_proxy.c
@@ -857,6 +857,5 @@ int main(void) {
  *  indent-tabs-mode: nil
  *  c-indent-level: 4
  *  c-basic-offset: 4
- *  tab-width: 4
  * End:
  */
diff --git a/python/libvir.c b/python/libvir.c
index fe650e8..8ad1122 100644
--- a/python/libvir.c
+++ b/python/libvir.c
@@ -1464,6 +1464,5 @@ initcygvirtmod
  *  indent-tabs-mode: nil
  *  c-indent-level: 4
  *  c-basic-offset: 4
- *  tab-width: 4
  * End:
  */
...

---------------------------------------------------------------------
Then remove the offending in-indentation-only TABs from files
with "indent-tabs-mode: nil":

	In a file with "indent-tabs-mode: nil", no TABs for indentation.
	Convert offenders via "expand --initial".
	* Makefile.maint (sc_TAB_in_indentation): New rule to prevent regression.
	* proxy/libvirt_proxy.c: Convert indentation TABs to spaces.
	* python/libvir.c, src/buf.c: src/conf.c, src/conf.h: Likewise.
	* src/driver.h, src/internal.h, src/libvirt.c, src/lxc_conf.c: Likewise.
	* src/openvz_driver.h, src/proxy_internal.c: Likewise.
	* src/proxy_internal.h, src/qemu_conf.c, src/qemu_driver.c: Likewise.
	* src/qparams.h, src/remote_internal.c, src/stats_linux.h: Likewise.
	* src/util.c, src/virsh.c, src/virterror.c, src/xen_internal.c: Likewise.
	* src/xen_unified.c, src/xen_unified.h, src/xend_internal.c: Likewise.
	* src/xm_internal.c, src/xml.c, src/xs_internal.c: Likewise.
	* tests/nodeinfotest.c, tests/sexpr2xmltest.c: Likewise.

Signed-off-by: Jim Meyering <meyering redhat com>
---
 Makefile.maint        |    9 +
 proxy/libvirt_proxy.c |  652 ++++++++++++++++++++++++------------------------
 python/libvir.c       |   34 ++--
 src/buf.c             |   56 ++--
 src/conf.c            |  324 ++++++++++++------------
 src/conf.h            |   16 +-
 src/driver.h          |  260 ++++++++++----------
 src/internal.h        |   18 +-
 src/libvirt.c         |  174 +++++++-------
 src/lxc_conf.c        |   34 ++--
 src/openvz_driver.h   |   12 +-
 src/proxy_internal.c  |  244 +++++++++---------
 src/proxy_internal.h  |   26 +-
 src/qemu_conf.c       |    2 +-
 src/qemu_driver.c     |   10 +-
 src/qparams.h         |    2 +-
 src/remote_internal.c |    8 +-
 src/stats_linux.h     |    6 +-
 src/util.c            |    8 +-
 src/virsh.c           |   66 +++---
 src/virterror.c       |  356 ++++++++++++++--------------
 src/xen_internal.c    |  212 ++++++++--------
 src/xen_unified.c     |   52 ++--
 src/xen_unified.h     |   74 +++---
 src/xend_internal.c   |   88 ++++----
 src/xm_internal.c     |   50 ++--
 src/xml.c             |   12 +-
 src/xs_internal.c     |   90 ++++----
 tests/nodeinfotest.c  |    2 +-
 tests/sexpr2xmltest.c |  166 +++++++-------
 30 files changed, 1536 insertions(+), 1527 deletions(-)

diff --git a/Makefile.maint b/Makefile.maint
index fc21bf4..a22c0f5 100644
--- a/Makefile.maint
+++ b/Makefile.maint
@@ -296,6 +296,15 @@ sc_prohibit_tab_width_4:
 	  { echo '$(ME): found bogus "tab-width:" directive(s)'		\
 		1>&2; exit 1; } || :

+# Ensure that each file containing an "indent-tabs-mode: nil"
+# directive uses no TABs for indentation.
+sc_TAB_in_indentation:
+	@grep -lE '^ *	' /dev/null					\
+	  $$(grep -lE '^$(sp)*\*$(sp)*indent-tabs-mode: nil$$'		\
+	     $$($(VC_LIST_EXCEPT))) &&					\
+	  { echo '$(ME): found TAB(s) used for indentation'		\
+		1>&2; exit 1; } || :
+
 # Match lines like the following, but where there is only one space
 # between the options and the description:
 #   -D, --all-repeated[=delimit-method]  print all duplicate lines\n
diff --git a/proxy/libvirt_proxy.c b/proxy/libvirt_proxy.c
index b28a3bc..6be3ad8 100644
--- a/proxy/libvirt_proxy.c
+++ b/proxy/libvirt_proxy.c
@@ -93,10 +93,10 @@ proxyInitXen(void) {
         return(-1);
     } else {
         ret = xenHypervisorGetVersion(conn, &xenVersion);
-	if (ret != 0) {
-	    fprintf(stderr, "Failed to get Xen hypervisor version\n");
-	    return(-1);
-	}
+        if (ret != 0) {
+            fprintf(stderr, "Failed to get Xen hypervisor version\n");
+            return(-1);
+        }
     }
     ret = xenDaemonOpen_unix(conn, "/var/lib/xend/xend-socket");
     if (ret < 0) {
@@ -110,12 +110,12 @@ proxyInitXen(void) {
     }
     ret = xenDaemonGetVersion(conn, &xenVersion2);
     if (ret != 0) {
-	fprintf(stderr, "Failed to get Xen daemon version\n");
-	return(-1);
+        fprintf(stderr, "Failed to get Xen daemon version\n");
+        return(-1);
     }
     if (debug)
         fprintf(stderr, "Connected to hypervisor %lu and daemon %lu\n",
-	        xenVersion, xenVersion2);
+                xenVersion, xenVersion2);
     if (xenVersion2 > xenVersion)
         xenVersion = xenVersion2;
     return(0);
...


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