[Qemu-devel] [PATCH 0/4] updates to coding style and checkpatch

[Qemu-devel] [PATCH 0/4] updates to coding style and checkpatch

[Paolo Bonzini]

The first two patches are a resend of a previous proposal sent to qemu-devel.
The last two are new.

Paolo

Paolo Bonzini (4):
  CODING_STYLE: update mixed declaration rules
  CODING_STYLE, checkpatch: update line length rules
  checkpatch: adapt some tests to QEMU
  checkpatch: remove tests that are not relevant outside the kernel

 CODING_STYLE          |  26 ++-
 scripts/checkpatch.pl | 564 ++++++++------------------------------------------
 2 files changed, 105 insertions(+), 485 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/4] CODING_STYLE: update mixed declaration rules

[Paolo Bonzini]

Mixed declarations also do exist at the top of #ifdef blocks.
Reluctantly allow this particular usage and suggest an alternative.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 CODING_STYLE | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/CODING_STYLE b/CODING_STYLE
index d46cfa5..3c6978f 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -87,10 +87,15 @@ Furthermore, it is the QEMU coding style.
 
 5. Declarations
 
-Mixed declarations (interleaving statements and declarations within blocks)
-are not allowed; declarations should be at the beginning of blocks.  In other
-words, the code should not generate warnings if using GCC's
--Wdeclaration-after-statement option.
+Mixed declarations (interleaving statements and declarations within
+blocks) are generally not allowed; declarations should be at the beginning
+of blocks.
+
+Every now and then, an exception is made for declarations inside a
+#ifdef or #ifndef block: if the code looks nicer, such declarations can
+be placed at the top of the block even if there are statements above.
+On the other hand, however, it's often best to move that #ifdef/#ifndef
+block to a separate function altogether.
 
 6. Conditional statements
 
-- 
2.4.3

[Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Paolo Bonzini]

Line lengths above 80 characters do exist.  They are rare, but
they happen from time to time.  An ignored rule is worse than an
exception to the rule, so do the latter.

Based on remarks from the list, make the preferred line length
slightly lower than 80 characters, to account for extra characters
in unified diffs (including three-way diffs) and for email quoting.

Checkpatch has some code to detect doc comments that doesn't apply
to QEMU; the usual limits apply even for doc comments in our case.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 CODING_STYLE          | 13 ++++++++++---
 scripts/checkpatch.pl | 21 +++++++++++++++------
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/CODING_STYLE b/CODING_STYLE
index 3c6978f..34d5526 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -31,14 +31,21 @@ Do not leave whitespace dangling off the ends of lines.
 
 2. Line width
 
-Lines are 80 characters; not longer.
+Lines should be 76 characters; try not to make them longer.
+
+Sometimes it is hard to do, especially when dealing with QEMU subsystems
+that use long function or symbol names.  Even in that case, do not make
+lines much longer than 76 characters.
 
 Rationale:
  - Some people like to tile their 24" screens with a 6x4 matrix of 80x24
-   xterms and use vi in all of them.  The best way to punish them is to
-   let them keep doing it.
+   xterms and use vi in all of them.  They also examine diffs (and three-way
+   diffs) on an 80-column terminal, accounting for two extra characters.
+   The best way to punish them is to let them keep doing it.
  - Code and especially patches is much more readable if limited to a sane
    line length.  Eighty is traditional.
+ - The four-space indentation makes the most common excuse ("But look
+   at all that white space on the left!") moot.
  - It is the QEMU coding style.
 
 3. Naming
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7f0aae9..bc32d8f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1466,14 +1466,23 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|cpp|s|S|pl|sh)$/);
 
-#80 column limit
-		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
-		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
+#90 column limit
+		if ($line =~ /^\+/ &&
 		    !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ ||
-		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
-		    $length > 80)
+		    !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/))
 		{
-			WARN("line over 80 characters\n" . $herecurr);
+			if ($length > 90) {
+				ERROR("line over 90 characters\n" . $herecurr);
+			} elsif ($length > 76 && !($rawline =~ /^\+ \* /)) {
+				# The BSD license blurb has 80 character lines.
+				# Avoid warning on cut-and-pasted license text.
+				WARN("line over 76 characters\n" . $herecurr);
+			} elsif ($length > 80) {
+				# Do not confuse the user by introducing yet
+				# another limit (80 characters).  Technically,
+				# this line *is* over 76 characters.
+				WARN("line over 76 characters\n" . $herecurr);
+			}
 		}
 
 # check for spaces before a quoted newline
-- 
2.4.3

[Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Paolo Bonzini]

Mostly change severity levels, but some tests can also be adjusted to refer
to QEMU APIs or data structures.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/checkpatch.pl | 141 +++++++++++++++++++++-----------------------------
 1 file changed, 60 insertions(+), 81 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bc32d8f..f234c2f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -152,33 +152,18 @@ our $Sparse	= qr{
 		}x;
 
 # Notes to $Attribute:
-# We need \b after 'init' otherwise 'initconst' will cause a false positive in a check
 our $Attribute	= qr{
 			const|
-			__percpu|
-			__nocast|
-			__safe|
-			__bitwise__|
-			__packed__|
-			__packed2__|
-			__naked|
-			__maybe_unused|
-			__always_unused|
-			__noreturn|
-			__used|
-			__cold|
-			__noclone|
-			__deprecated|
-			__read_mostly|
-			__kprobes|
-			__(?:mem|cpu|dev|)(?:initdata|initconst|init\b)|
-			____cacheline_aligned|
-			____cacheline_aligned_in_smp|
-			____cacheline_internodealigned_in_smp|
-			__weak
+			volatile|
+			QEMU_NORETURN|
+			QEMU_WARN_UNUSED_RESULT|
+			QEMU_SENTINEL|
+			QEMU_ARTIFICIAL|
+			QEMU_PACKED|
+			GCC_FMT_ATTR
 		  }x;
 our $Modifier;
-our $Inline	= qr{inline|__always_inline|noinline};
+our $Inline	= qr{inline};
 our $Member	= qr{->$Ident|\.$Ident|\[[^]]*\]};
 our $Lval	= qr{$Ident(?:$Member)*};
 
@@ -1367,7 +1352,7 @@ sub process {
 # Check for incorrect file permissions
 		if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {
 			my $permhere = $here . "FILE: $realfile\n";
-			if ($realfile =~ /(Makefile|Kconfig|\.c|\.cpp|\.h|\.S|\.tmpl)$/) {
+			if ($realfile =~ /(\bMakefile(?:\.objs)?|\.c|\.cc|\.cpp|\.h|\.mak|\.[sS])$/) {
 				ERROR("do not set execute permissions for source files\n" . $permhere);
 			}
 		}
@@ -1956,9 +1941,14 @@ sub process {
 			ERROR("open brace '{' following $1 go on the same line\n" . $hereprev);
 		}
 
+# ... however, open braces on typedef lines should be avoided.
+		if ($line =~ /^.\s*typedef\s+(enum|union|struct)(?:\s+$Ident\b)?.*[^;]$/) {
+			ERROR("typedefs should be separate from struct declaration\n" . $herecurr);
+		}
+
 # missing space after union, struct or enum definition
 		if ($line =~ /^.\s*(?:typedef\s+)?(enum|union|struct)(?:\s+$Ident)?(?:\s+$Ident)?[=\{]/) {
-		    WARN("missing space after $1 definition\n" . $herecurr);
+		    ERROR("missing space after $1 definition\n" . $herecurr);
 		}
 
 # check for spacing round square brackets; allowed:
@@ -2276,7 +2266,7 @@ sub process {
 		if ($line =~ /^.\s*return\s*(E[A-Z]*)\s*;/) {
 			my $name = $1;
 			if ($name ne 'EOF' && $name ne 'ERROR') {
-				CHK("return of an errno should typically be -ve (return -$1)\n" . $herecurr);
+				WARN("return of an errno should typically be -ve (return -$1)\n" . $herecurr);
 			}
 		}
 
@@ -2680,15 +2670,15 @@ sub process {
 
 # warn about #if 0
 		if ($line =~ /^.\s*\#\s*if\s+0\b/) {
-			CHK("if this code is redundant consider removing it\n" .
+			WARN("if this code is redundant consider removing it\n" .
 				$herecurr);
 		}
 
-# check for needless kfree() checks
+# check for needless g_free() checks
 		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
 			my $expr = $1;
-			if ($line =~ /\bkfree\(\Q$expr\E\);/) {
-				WARN("kfree(NULL) is safe this check is probably not required\n" . $hereprev);
+			if ($line =~ /\bg_free\(\Q$expr\E\);/) {
+				WARN("g_free(NULL) is safe this check is probably not required\n" . $hereprev);
 			}
 		}
 # check for needless usb_free_urb() checks
@@ -2735,14 +2725,16 @@ sub process {
 			}
 		}
 # check for memory barriers without a comment.
-		if ($line =~ /\b(mb|rmb|wmb|read_barrier_depends|smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) {
+		if ($line =~ /\b(smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) {
 			if (!ctx_has_comment($first_line, $linenr)) {
-				CHK("memory barrier without comment\n" . $herecurr);
+				WARN("memory barrier without comment\n" . $herecurr);
 			}
 		}
 # check of hardware specific defines
-		if ($line =~ m@^.\s*\#\s*if.*\b(__i386__|__powerpc64__|__sun__|__s390x__)\b@ && $realfile !~ m@include/asm-@) {
-			CHK("architecture specific defines should be avoided\n" .  $herecurr);
+# we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases
+# where they might be necessary.
+		if ($line =~ m@^.\s*\#\s*if.*\b__@) {
+			WARN("architecture specific defines should be avoided\n" .  $herecurr);
 		}
 
 # Check that the storage class is at the beginning of a declaration
@@ -2803,9 +2795,13 @@ sub process {
 			}
 		}
 
-# check for pointless casting of kmalloc return
-		if ($line =~ /\*\s*\)\s*k[czm]alloc\b/) {
-			WARN("unnecessary cast may hide bugs, see http://c-faq.com/malloc/mallocnocast.html\n" . $herecurr);
+# check for pointless casting of g_malloc return
+		if ($line =~ /\*\s*\)\s*g_(try)?(m|re)alloc(0?)(_n)?\b/) {
+			if ($2 == 'm') {
+				WARN("unnecessary cast may hide bugs, use g_$1new$3 instead\n" . $herecurr);
+			} else {
+				WARN("unnecessary cast may hide bugs, use g_$1renew$3 instead\n" . $herecurr);
+			}
 		}
 
 # check for gcc specific __FUNCTION__
@@ -2822,54 +2818,37 @@ sub process {
 			WARN("consider using a completion\n" . $herecurr);
 
 		}
-# recommend strict_strto* over simple_strto*
-		if ($line =~ /\bsimple_(strto.*?)\s*\(/) {
-			WARN("consider using strict_$1 in preference to simple_$1\n" . $herecurr);
+# recommend qemu_strto* over strto*
+		if ($line =~ /\b(strto.*?)\s*\(/) {
+			WARN("consider using qemu_$1 in preference to $1\n" . $herecurr);
 		}
-# check for __initcall(), use device_initcall() explicitly please
-		if ($line =~ /^.\s*__initcall\s*\(/) {
-			WARN("please use device_initcall() instead of __initcall()\n" . $herecurr);
+# check for module_init(), use category-specific init macros explicitly please
+		if ($line =~ /^module_init\s*\(/) {
+			WARN("please use block_init(), type_init() etc. instead of module_init()\n" . $herecurr);
 		}
 # check for various ops structs, ensure they are const.
-		my $struct_ops = qr{acpi_dock_ops|
-				address_space_operations|
-				backlight_ops|
-				block_device_operations|
-				dentry_operations|
-				dev_pm_ops|
-				dma_map_ops|
-				extent_io_ops|
-				file_lock_operations|
-				file_operations|
-				hv_ops|
-				ide_dma_ops|
-				intel_dvo_dev_ops|
-				item_operations|
-				iwl_ops|
-				kgdb_arch|
-				kgdb_io|
-				kset_uevent_ops|
-				lock_manager_operations|
-				microcode_ops|
-				mtrr_ops|
-				neigh_ops|
-				nlmsvc_binding|
-				pci_raw_ops|
-				pipe_buf_operations|
-				platform_hibernation_ops|
-				platform_suspend_ops|
-				proto_ops|
-				rpc_pipe_ops|
-				seq_operations|
-				snd_ac97_build_ops|
-				soc_pcmcia_socket_ops|
-				stacktrace_ops|
-				sysfs_ops|
-				tty_operations|
-				usb_mon_operations|
-				wd_ops}x;
+		my $struct_ops = qr{AIOCBInfo|
+				BdrvActionOps|
+				BlockDevOps|
+				BlockJobDriver|
+				DisplayChangeListenerOps|
+				GraphicHwOps|
+				IDEDMAOps|
+				KVMCapabilityInfo|
+				MemoryRegionIOMMUOps|
+				MemoryRegionOps|
+				MemoryRegionPortio|
+				QEMUFileOps|
+				SCSIBusInfo|
+				SCSIReqOps|
+				Spice[A-Z][a-zA-Z0-9]*Interface|
+				TPMDriverOps|
+				USBDesc[A-Z][a-zA-Z0-9]*|
+				VhostOps|
+				VMStateDescription|
+				VMStateInfo}x;
 		if ($line !~ /\bconst\b/ &&
-		    $line =~ /\bstruct\s+($struct_ops)\b/) {
+		    $line =~ /\b($struct_ops)\b/) {
 			WARN("struct $1 should normally be const\n" .
 				$herecurr);
 		}
-- 
2.4.3

[Qemu-devel] [PATCH 4/4] checkpatch: remove tests that are not relevant outside the kernel

[Paolo Bonzini]

Fully removing Sparse support requires more invasive changes.  Only
remove the really kernel-specific parts such as address space names.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/checkpatch.pl | 396 ++------------------------------------------------
 1 file changed, 10 insertions(+), 386 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f234c2f..3ef6abd 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -141,14 +141,7 @@ our $Ident	= qr{
 		}x;
 our $Storage	= qr{extern|static|asmlinkage};
 our $Sparse	= qr{
-			__user|
-			__kernel|
-			__force|
-			__iomem|
-			__must_check|
-			__init_refok|
-			__kprobes|
-			__ref
+			__force
 		}x;
 
 # Notes to $Attribute:
@@ -200,14 +193,6 @@ our $typeTypedefs = qr{(?x:
         | QEMUBH                            # all uppercase
 )};
 
-our $logFunctions = qr{(?x:
-	printk|
-	pr_(debug|dbg|vdbg|devel|info|warning|err|notice|alert|crit|emerg|cont)|
-	(dev|netdev|netif)_(printk|dbg|vdbg|info|warn|err|notice|alert|crit|emerg|WARN)|
-	WARN|
-	panic
-)};
-
 our @typeList = (
 	qr{void},
 	qr{(?:unsigned\s+)?char},
@@ -228,20 +213,20 @@ our @typeList = (
 	qr{${Ident}_handler},
 	qr{${Ident}_handler_fn},
 );
+
+# This can be modified by sub possible.  Since it can be empty, be careful
+# about regexes that always match, because they can cause infinite loops.
 our @modifierList = (
-	qr{fastcall},
 );
 
-our $allowed_asm_includes = qr{(?x:
-	irq|
-	memory
-)};
-# memory.h: ARM has a custom one
-
 sub build_types {
-	my $mods = "(?x:  \n" . join("|\n  ", @modifierList) . "\n)";
 	my $all = "(?x:  \n" . join("|\n  ", @typeList) . "\n)";
-	$Modifier	= qr{(?:$Attribute|$Sparse|$mods)};
+	if (@modifierList > 0) {
+		my $mods = "(?x:  \n" . join("|\n  ", @modifierList) . "\n)";
+		$Modifier = qr{(?:$Attribute|$Sparse|$mods)};
+	} else {
+		$Modifier = qr{(?:$Attribute|$Sparse)};
+	}
 	$NonptrType	= qr{
 			(?:$Modifier\s+|const\s+)*
 			(?:
@@ -262,27 +247,6 @@ build_types();
 
 $chk_signoff = 0 if ($file);
 
-my @dep_includes = ();
-my @dep_functions = ();
-my $removal = "Documentation/feature-removal-schedule.txt";
-if ($tree && -f "$root/$removal") {
-	open(my $REMOVE, '<', "$root/$removal") ||
-				die "$P: $removal: open failed - $!\n";
-	while (<$REMOVE>) {
-		if (/^Check:\s+(.*\S)/) {
-			for my $entry (split(/[, ]+/, $1)) {
-				if ($entry =~ m@include/(.*)@) {
-					push(@dep_includes, $1);
-
-				} elsif ($entry !~ m@/@) {
-					push(@dep_functions, $entry);
-				}
-			}
-		}
-	}
-	close($REMOVE);
-}
-
 my @rawlines = ();
 my @lines = ();
 my $vname;
@@ -1112,33 +1076,6 @@ sub CHK {
 	}
 }
 
-sub check_absolute_file {
-	my ($absolute, $herecurr) = @_;
-	my $file = $absolute;
-
-	##print "absolute<$absolute>\n";
-
-	# See if any suffix of this path is a path within the tree.
-	while ($file =~ s@^[^/]*/@@) {
-		if (-f "$root/$file") {
-			##print "file<$file>\n";
-			last;
-		}
-	}
-	if (! -f _)  {
-		return 0;
-	}
-
-	# It is, so see if the prefix is acceptable.
-	my $prefix = $absolute;
-	substr($prefix, -length($file)) = '';
-
-	##print "prefix<$prefix>\n";
-	if ($prefix ne ".../") {
-		WARN("use relative pathname instead of absolute in changelog text\n" . $herecurr);
-	}
-}
-
 sub process {
 	my $filename = shift;
 
@@ -1181,10 +1118,6 @@ sub process {
 	my %suppress_export;
 
 	# Pre-scan the patch sanitizing the lines.
-	# Pre-scan the patch looking for any __setup documentation.
-	#
-	my @setup_docs = ();
-	my $setup_docs = 0;
 
 	sanitise_line_reset();
 	my $line;
@@ -1192,13 +1125,6 @@ sub process {
 		$linenr++;
 		$line = $rawline;
 
-		if ($rawline=~/^\+\+\+\s+(\S+)/) {
-			$setup_docs = 0;
-			if ($1 =~ m@Documentation/kernel-parameters.txt$@) {
-				$setup_docs = 1;
-			}
-			#next;
-		}
 		if ($rawline=~/^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
 			$realline=$1-1;
 			if (defined $2) {
@@ -1257,10 +1183,6 @@ sub process {
 
 		#print "==>$rawline\n";
 		#print "-->$line\n";
-
-		if ($setup_docs && $line =~ /^\+/) {
-			push(@setup_docs, $line);
-		}
 	}
 
 	$prefix = '';
@@ -1335,9 +1257,6 @@ sub process {
 				WARN("patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
 			}
 
-			if ($realfile =~ m@^include/asm/@) {
-				ERROR("do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");
-			}
 			next;
 		}
 
@@ -1377,20 +1296,6 @@ sub process {
 				$herecurr) if (!$emitted_corrupt++);
 		}
 
-# Check for absolute kernel paths.
-		if ($tree) {
-			while ($line =~ m{(?:^|\s)(/\S*)}g) {
-				my $file = $1;
-
-				if ($file =~ m{^(.*?)(?::\d+)+:?$} &&
-				    check_absolute_file($1, $herecurr)) {
-					#
-				} else {
-					check_absolute_file($file, $herecurr);
-				}
-			}
-		}
-
 # UTF-8 regex found at http://www.w3.org/International/questions/qa-forms-utf-8.en.php
 		if (($realfile =~ /^$/ || $line =~ /^\+/) &&
 		    $rawline !~ m/^$UTF8*$/) {
@@ -1417,43 +1322,11 @@ sub process {
 			$rpt_cleaners = 1;
 		}
 
-# check for Kconfig help text having a real description
-# Only applies when adding the entry originally, after that we do not have
-# sufficient context to determine whether it is indeed long enough.
-		if ($realfile =~ /Kconfig/ &&
-		    $line =~ /\+\s*(?:---)?help(?:---)?$/) {
-			my $length = 0;
-			my $cnt = $realcnt;
-			my $ln = $linenr + 1;
-			my $f;
-			my $is_end = 0;
-			while ($cnt > 0 && defined $lines[$ln - 1]) {
-				$f = $lines[$ln - 1];
-				$cnt-- if ($lines[$ln - 1] !~ /^-/);
-				$is_end = $lines[$ln - 1] =~ /^\+/;
-				$ln++;
-
-				next if ($f =~ /^-/);
-				$f =~ s/^.//;
-				$f =~ s/#.*//;
-				$f =~ s/^\s+//;
-				next if ($f =~ /^$/);
-				if ($f =~ /^\s*config\s/) {
-					$is_end = 1;
-					last;
-				}
-				$length++;
-			}
-			WARN("please write a paragraph that describes the config symbol fully\n" . $herecurr) if ($is_end && $length < 4);
-			#print "is_end<$is_end> length<$length>\n";
-		}
-
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|cpp|s|S|pl|sh)$/);
 
 #90 column limit
 		if ($line =~ /^\+/ &&
-		    !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ ||
 		    !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/))
 		{
 			if ($length > 90) {
@@ -1480,18 +1353,6 @@ sub process {
 			WARN("adding a line without newline at end of file\n" . $herecurr);
 		}
 
-# Blackfin: use hi/lo macros
-		if ($realfile =~ m@arch/blackfin/.*\.S$@) {
-			if ($line =~ /\.[lL][[:space:]]*=.*&[[:space:]]*0x[fF][fF][fF][fF]/) {
-				my $herevet = "$here\n" . cat_vet($line) . "\n";
-				ERROR("use the LO() macro, not (... & 0xFFFF)\n" . $herevet);
-			}
-			if ($line =~ /\.[hH][[:space:]]*=.*>>[[:space:]]*16/) {
-				my $herevet = "$here\n" . cat_vet($line) . "\n";
-				ERROR("use the HI() macro, not (... >> 16)\n" . $herevet);
-			}
-		}
-
 # check we are in a valid source file C or perl if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
 
@@ -1510,16 +1371,6 @@ sub process {
 			WARN("CVS style keyword markers, these will _not_ be updated\n". $herecurr);
 		}
 
-# Blackfin: don't use __builtin_bfin_[cs]sync
-		if ($line =~ /__builtin_bfin_csync/) {
-			my $herevet = "$here\n" . cat_vet($line) . "\n";
-			ERROR("use the CSYNC() macro in asm/blackfin.h\n" . $herevet);
-		}
-		if ($line =~ /__builtin_bfin_ssync/) {
-			my $herevet = "$here\n" . cat_vet($line) . "\n";
-			ERROR("use the SSYNC() macro in asm/blackfin.h\n" . $herevet);
-		}
-
 # Check for potential 'bare' types
 		my ($stat, $cond, $line_nr_next, $remain_next, $off_next,
 		    $realline_next);
@@ -1803,50 +1654,6 @@ sub process {
 		$line =~ s@//.*@@;
 		$opline =~ s@//.*@@;
 
-# EXPORT_SYMBOL should immediately follow the thing it is exporting, consider
-# the whole statement.
-#print "APW <$lines[$realline_next - 1]>\n";
-		if (defined $realline_next &&
-		    exists $lines[$realline_next - 1] &&
-		    !defined $suppress_export{$realline_next} &&
-		    ($lines[$realline_next - 1] =~ /EXPORT_SYMBOL.*\((.*)\)/ ||
-		     $lines[$realline_next - 1] =~ /EXPORT_UNUSED_SYMBOL.*\((.*)\)/)) {
-			# Handle definitions which produce identifiers with
-			# a prefix:
-			#   XXX(foo);
-			#   EXPORT_SYMBOL(something_foo);
-			my $name = $1;
-			if ($stat =~ /^.([A-Z_]+)\s*\(\s*($Ident)/ &&
-			    $name =~ /^${Ident}_$2/) {
-#print "FOO C name<$name>\n";
-				$suppress_export{$realline_next} = 1;
-
-			} elsif ($stat !~ /(?:
-				\n.}\s*$|
-				^.DEFINE_$Ident\(\Q$name\E\)|
-				^.DECLARE_$Ident\(\Q$name\E\)|
-				^.LIST_HEAD\(\Q$name\E\)|
-				^.(?:$Storage\s+)?$Type\s*\(\s*\*\s*\Q$name\E\s*\)\s*\(|
-				\b\Q$name\E(?:\s+$Attribute)*\s*(?:;|=|\[|\()
-			    )/x) {
-#print "FOO A<$lines[$realline_next - 1]> stat<$stat> name<$name>\n";
-				$suppress_export{$realline_next} = 2;
-			} else {
-				$suppress_export{$realline_next} = 1;
-			}
-		}
-		if (!defined $suppress_export{$linenr} &&
-		    $prevline =~ /^.\s*$/ &&
-		    ($line =~ /EXPORT_SYMBOL.*\((.*)\)/ ||
-		     $line =~ /EXPORT_UNUSED_SYMBOL.*\((.*)\)/)) {
-#print "FOO B <$lines[$linenr - 1]>\n";
-			$suppress_export{$linenr} = 2;
-		}
-		if (defined $suppress_export{$linenr} &&
-		    $suppress_export{$linenr} == 2) {
-			WARN("EXPORT_SYMBOL(foo); should immediately follow its function/variable\n" . $herecurr);
-		}
-
 # check for global initialisers.
 		if ($line =~ /^.$Type\s*$Ident\s*(?:\s+$Modifier)*\s*=\s*(0|NULL|false)\s*;/) {
 			ERROR("do not initialise globals to 0 or NULL\n" .
@@ -1894,40 +1701,6 @@ sub process {
 			}
 		}
 
-# # no BUG() or BUG_ON()
-# 		if ($line =~ /\b(BUG|BUG_ON)\b/) {
-# 			print "Try to use WARN_ON & Recovery code rather than BUG() or BUG_ON()\n";
-# 			print "$herecurr";
-# 			$clean = 0;
-# 		}
-
-		if ($line =~ /\bLINUX_VERSION_CODE\b/) {
-			WARN("LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged\n" . $herecurr);
-		}
-
-# printk should use KERN_* levels.  Note that follow on printk's on the
-# same line do not need a level, so we use the current block context
-# to try and find and validate the current printk.  In summary the current
-# printk includes all preceding printk's which have no newline on the end.
-# we assume the first bad printk is the one to report.
-		if ($line =~ /\bprintk\((?!KERN_)\s*"/) {
-			my $ok = 0;
-			for (my $ln = $linenr - 1; $ln >= $first_line; $ln--) {
-				#print "CHECK<$lines[$ln - 1]\n";
-				# we have a preceding printk if it ends
-				# with "\n" ignore it, else it is to blame
-				if ($lines[$ln - 1] =~ m{\bprintk\(}) {
-					if ($rawlines[$ln - 1] !~ m{\\n"}) {
-						$ok = 1;
-					}
-					last;
-				}
-			}
-			if ($ok == 0) {
-				WARN("printk() should include KERN_ facility level\n" . $herecurr);
-			}
-		}
-
 # function brace can't be on same line, except for #defines of do while,
 # or if closed on same line
 		if (($line=~/$Type\s*$Ident\(.*\).*\s{/) and
@@ -2189,26 +1962,6 @@ sub process {
 			}
 		}
 
-# check for multiple assignments
-		if ($line =~ /^.\s*$Lval\s*=\s*$Lval\s*=(?!=)/) {
-			CHK("multiple assignments should be avoided\n" . $herecurr);
-		}
-
-## # check for multiple declarations, allowing for a function declaration
-## # continuation.
-## 		if ($line =~ /^.\s*$Type\s+$Ident(?:\s*=[^,{]*)?\s*,\s*$Ident.*/ &&
-## 		    $line !~ /^.\s*$Type\s+$Ident(?:\s*=[^,{]*)?\s*,\s*$Type\s*$Ident.*/) {
-##
-## 			# Remove any bracketed sections to ensure we do not
-## 			# falsly report the parameters of functions.
-## 			my $ln = $line;
-## 			while ($ln =~ s/\([^\(\)]*\)//g) {
-## 			}
-## 			if ($ln =~ /,/) {
-## 				WARN("declaring multiple variables together should be avoided\n" . $herecurr);
-## 			}
-## 		}
-
 #need space before brace following if, while, etc
 		if (($line =~ /\(.*\){/ && $line !~ /\($Type\){/) ||
 		    $line =~ /do{/) {
@@ -2397,22 +2150,6 @@ sub process {
 			WARN("Whitepspace after \\ makes next lines useless\n" . $herecurr);
 		}
 
-#warn if <asm/foo.h> is #included and <linux/foo.h> is available (uses RAW line)
-		if ($tree && $rawline =~ m{^.\s*\#\s*include\s*\<asm\/(.*)\.h\>}) {
-			my $file = "$1.h";
-			my $checkfile = "include/linux/$file";
-			if (-f "$root/$checkfile" &&
-			    $realfile ne $checkfile &&
-			    $1 !~ /$allowed_asm_includes/)
-			{
-				if ($realfile =~ m{^arch/}) {
-					CHK("Consider using #include <linux/$file> instead of <asm/$file>\n" . $herecurr);
-				} else {
-					WARN("Use #include <linux/$file> instead of <asm/$file>\n" . $herecurr);
-				}
-			}
-		}
-
 # multi-statement macros should be enclosed in a do while loop, grab the
 # first statement and ensure its the whole macro if its not enclosed
 # in a known good container
@@ -2507,15 +2244,6 @@ sub process {
 			}
 		}
 
-# make sure symbols are always wrapped with VMLINUX_SYMBOL() ...
-# all assignments may have only one of the following with an assignment:
-#	.
-#	ALIGN(...)
-#	VMLINUX_SYMBOL(...)
-		if ($realfile eq 'vmlinux.lds.h' && $line =~ /(?:(?:^|\s)$Ident\s*=|=\s*$Ident(?:\s|$))/) {
-			WARN("vmlinux.lds.h needs VMLINUX_SYMBOL() around C-visible symbols\n" . $herecurr);
-		}
-
 # check for missing bracing round if etc
 		if ($line =~ /(^.*)\bif\b/ && $line !~ /\#\s*if/) {
 			my ($level, $endln, @chunks) =
@@ -2643,31 +2371,12 @@ sub process {
 			}
 		}
 
-# don't include deprecated include files (uses RAW line)
-		for my $inc (@dep_includes) {
-			if ($rawline =~ m@^.\s*\#\s*include\s*\<$inc>@) {
-				ERROR("Don't use <$inc>: see Documentation/feature-removal-schedule.txt\n" . $herecurr);
-			}
-		}
-
-# don't use deprecated functions
-		for my $func (@dep_functions) {
-			if ($line =~ /\b$func\b/) {
-				ERROR("Don't use $func(): see Documentation/feature-removal-schedule.txt\n" . $herecurr);
-			}
-		}
-
 # no volatiles please
 		my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
 		if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
 			WARN("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);
 		}
 
-# SPIN_LOCK_UNLOCKED & RW_LOCK_UNLOCKED are deprecated
-		if ($line =~ /\b(SPIN_LOCK_UNLOCKED|RW_LOCK_UNLOCKED)/) {
-			ERROR("Use of $1 is deprecated: see Documentation/spinlocks.txt\n" . $herecurr);
-		}
-
 # warn about #if 0
 		if ($line =~ /^.\s*\#\s*if\s+0\b/) {
 			WARN("if this code is redundant consider removing it\n" .
@@ -2681,28 +2390,6 @@ sub process {
 				WARN("g_free(NULL) is safe this check is probably not required\n" . $hereprev);
 			}
 		}
-# check for needless usb_free_urb() checks
-		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
-			my $expr = $1;
-			if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
-				WARN("usb_free_urb(NULL) is safe this check is probably not required\n" . $hereprev);
-			}
-		}
-
-# prefer usleep_range over udelay
-		if ($line =~ /\budelay\s*\(\s*(\w+)\s*\)/) {
-			# ignore udelay's < 10, however
-			if (! (($1 =~ /(\d+)/) && ($1 < 10)) ) {
-				CHK("usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt\n" . $line);
-			}
-		}
-
-# warn about unexpectedly long msleep's
-		if ($line =~ /\bmsleep\s*\((\d+)\);/) {
-			if ($1 < 20) {
-				WARN("msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt\n" . $line);
-			}
-		}
 
 # warn about #ifdefs in C files
 #		if ($line =~ /^.\s*\#\s*if(|n)def/ && ($realfile =~ /\.c$/)) {
@@ -2715,15 +2402,6 @@ sub process {
 		if ($line =~ /^.\s*\#\s*(ifdef|ifndef|elif)\s\s+/) {
 			ERROR("exactly one space required after that #$1\n" . $herecurr);
 		}
-
-# check for spinlock_t definitions without a comment.
-		if ($line =~ /^.\s*(struct\s+mutex|spinlock_t)\s+\S+;/ ||
-		    $line =~ /^.\s*(DEFINE_MUTEX)\s*\(/) {
-			my $which = $1;
-			if (!ctx_has_comment($first_line, $linenr)) {
-				CHK("$1 definition without comment\n" . $herecurr);
-			}
-		}
 # check for memory barriers without a comment.
 		if ($line =~ /\b(smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) {
 			if (!ctx_has_comment($first_line, $linenr)) {
@@ -2749,11 +2427,6 @@ sub process {
 			ERROR("inline keyword should sit between storage class and type\n" . $herecurr);
 		}
 
-# Check for __inline__ and __inline, prefer inline
-		if ($line =~ /\b(__inline__|__inline)\b/) {
-			WARN("plain inline is preferred over $1\n" . $herecurr);
-		}
-
 # check for sizeof(&)
 		if ($line =~ /\bsizeof\s*\(\s*\&/) {
 			WARN("sizeof(& should be avoided\n" . $herecurr);
@@ -2786,15 +2459,6 @@ sub process {
 			WARN("externs should be avoided in .c files\n" .  $herecurr);
 		}
 
-# checks for new __setup's
-		if ($rawline =~ /\b__setup\("([^"]*)"/) {
-			my $name = $1;
-
-			if (!grep(/$name/, @setup_docs)) {
-				CHK("__setup appears un-documented -- check Documentation/kernel-parameters.txt\n" . $herecurr);
-			}
-		}
-
 # check for pointless casting of g_malloc return
 		if ($line =~ /\*\s*\)\s*g_(try)?(m|re)alloc(0?)(_n)?\b/) {
 			if ($2 == 'm') {
@@ -2809,15 +2473,6 @@ sub process {
 			WARN("__func__ should be used instead of gcc specific __FUNCTION__\n"  . $herecurr);
 		}
 
-# check for semaphores used as mutexes
-		if ($line =~ /^.\s*(DECLARE_MUTEX|init_MUTEX)\s*\(/) {
-			WARN("mutexes are preferred for single holder semaphores\n" . $herecurr);
-		}
-# check for semaphores used as mutexes
-		if ($line =~ /^.\s*init_MUTEX_LOCKED\s*\(/) {
-			WARN("consider using a completion\n" . $herecurr);
-
-		}
 # recommend qemu_strto* over strto*
 		if ($line =~ /\b(strto.*?)\s*\(/) {
 			WARN("consider using qemu_$1 in preference to $1\n" . $herecurr);
@@ -2853,18 +2508,6 @@ sub process {
 				$herecurr);
 		}
 
-# use of NR_CPUS is usually wrong
-# ignore definitions of NR_CPUS and usage to define arrays as likely right
-		if ($line =~ /\bNR_CPUS\b/ &&
-		    $line !~ /^.\s*\s*#\s*if\b.*\bNR_CPUS\b/ &&
-		    $line !~ /^.\s*\s*#\s*define\b.*\bNR_CPUS\b/ &&
-		    $line !~ /^.\s*$Declare\s.*\[[^\]]*NR_CPUS[^\]]*\]/ &&
-		    $line !~ /\[[^\]]*\.\.\.[^\]]*NR_CPUS[^\]]*\]/ &&
-		    $line !~ /\[[^\]]*NR_CPUS[^\]]*\.\.\.[^\]]*\]/)
-		{
-			WARN("usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc\n" . $herecurr);
-		}
-
 # check for %L{u,d,i} in strings
 		my $string;
 		while ($line =~ /(?:^|")([X\t]*)(?:"|$)/g) {
@@ -2876,25 +2519,6 @@ sub process {
 			}
 		}
 
-# whine mightly about in_atomic
-		if ($line =~ /\bin_atomic\s*\(/) {
-			if ($realfile =~ m@^drivers/@) {
-				ERROR("do not use in_atomic in drivers\n" . $herecurr);
-			} elsif ($realfile !~ m@^kernel/@) {
-				WARN("use of in_atomic() is incorrect outside core kernel code\n" . $herecurr);
-			}
-		}
-
-# check for lockdep_set_novalidate_class
-		if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
-		    $line =~ /__lockdep_no_validate__\s*\)/ ) {
-			if ($realfile !~ m@^kernel/lockdep@ &&
-			    $realfile !~ m@^include/linux/lockdep@ &&
-			    $realfile !~ m@^drivers/base/core@) {
-				ERROR("lockdep_no_validate class is reserved for device->mutex.\n" . $herecurr);
-			}
-		}
-
 # QEMU specific tests
 		if ($rawline =~ /\b(?:Qemu|QEmu)\b/) {
 			WARN("use QEMU instead of Qemu or QEmu\n" . $herecurr);
-- 
2.4.3

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Thomas Huth]

On 07/09/15 11:53, Paolo Bonzini wrote:
> Line lengths above 80 characters do exist.  They are rare, but
> they happen from time to time.  An ignored rule is worse than an
> exception to the rule, so do the latter.
> 
> Based on remarks from the list, make the preferred line length
> slightly lower than 80 characters, to account for extra characters
> in unified diffs (including three-way diffs) and for email quoting.
> 
> Checkpatch has some code to detect doc comments that doesn't apply
> to QEMU; the usual limits apply even for doc comments in our case.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  CODING_STYLE          | 13 ++++++++++---
>  scripts/checkpatch.pl | 21 +++++++++++++++------
>  2 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/CODING_STYLE b/CODING_STYLE
> index 3c6978f..34d5526 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -31,14 +31,21 @@ Do not leave whitespace dangling off the ends of lines.
>  
>  2. Line width
>  
> -Lines are 80 characters; not longer.
> +Lines should be 76 characters; try not to make them longer.
> +
> +Sometimes it is hard to do, especially when dealing with QEMU subsystems
> +that use long function or symbol names.  Even in that case, do not make
> +lines much longer than 76 characters.
>  
>  Rationale:
>   - Some people like to tile their 24" screens with a 6x4 matrix of 80x24
> -   xterms and use vi in all of them.  The best way to punish them is to
> -   let them keep doing it.
> +   xterms and use vi in all of them.  They also examine diffs (and three-way
> +   diffs) on an 80-column terminal, accounting for two extra characters.
> +   The best way to punish them is to let them keep doing it.
>   - Code and especially patches is much more readable if limited to a sane
>     line length.  Eighty is traditional.
> + - The four-space indentation makes the most common excuse ("But look
> +   at all that white space on the left!") moot.
>   - It is the QEMU coding style.
>  
>  3. Naming
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7f0aae9..bc32d8f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1466,14 +1466,23 @@ sub process {
>  # check we are in a valid source file if not then ignore this hunk
>  		next if ($realfile !~ /\.(h|c|cpp|s|S|pl|sh)$/);
>  
> -#80 column limit
> -		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
> -		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
> +#90 column limit
> +		if ($line =~ /^\+/ &&
>  		    !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ ||
> -		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
> -		    $length > 80)
> +		    !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/))
>  		{
> -			WARN("line over 80 characters\n" . $herecurr);
> +			if ($length > 90) {
> +				ERROR("line over 90 characters\n" . $herecurr);
> +			} elsif ($length > 76 && !($rawline =~ /^\+ \* /)) {
> +				# The BSD license blurb has 80 character lines.
> +				# Avoid warning on cut-and-pasted license text.
> +				WARN("line over 76 characters\n" . $herecurr);

Sorry, this is _ugly_! First, the limit in QEMU has always been 80
columns in the past, thus we've got tons of code that is written with 80
columns already, so you suddenly even can not even copy-n-paste / move
around code anymore that was valid before?!? That'll be a PITA since you
have to reformat all patches that move code around - and it will also be
a PITA for all reviewers since checking whether old code matches new
code becomes more difficult.

Second, I really don't like the idea of introducing exceptions here like
it is done with the BSD license check above ... that will only cause
confusion and "let's add another exception" situations later, I think.

Can't we simply keep the 80 columns limit instead, please? IMHO people
having trouble to view patches with 80 columns should simply be forced
to switch to proper tools instead...

 Thomas

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Paolo Bonzini]

On 07/09/2015 15:18, Thomas Huth wrote:
> On 07/09/15 11:53, Paolo Bonzini wrote:
>> Line lengths above 80 characters do exist.  They are rare, but
>> they happen from time to time.  An ignored rule is worse than an
>> exception to the rule, so do the latter.
>>
>> Based on remarks from the list, make the preferred line length
>> slightly lower than 80 characters, to account for extra characters
>> in unified diffs (including three-way diffs) and for email quoting.
>>
>> Checkpatch has some code to detect doc comments that doesn't apply
>> to QEMU; the usual limits apply even for doc comments in our case.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  CODING_STYLE          | 13 ++++++++++---
>>  scripts/checkpatch.pl | 21 +++++++++++++++------
>>  2 files changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/CODING_STYLE b/CODING_STYLE
>> index 3c6978f..34d5526 100644
>> --- a/CODING_STYLE
>> +++ b/CODING_STYLE
>> @@ -31,14 +31,21 @@ Do not leave whitespace dangling off the ends of lines.
>>  
>>  2. Line width
>>  
>> -Lines are 80 characters; not longer.
>> +Lines should be 76 characters; try not to make them longer.
>> +
>> +Sometimes it is hard to do, especially when dealing with QEMU subsystems
>> +that use long function or symbol names.  Even in that case, do not make
>> +lines much longer than 76 characters.
>>  
>>  Rationale:
>>   - Some people like to tile their 24" screens with a 6x4 matrix of 80x24
>> -   xterms and use vi in all of them.  The best way to punish them is to
>> -   let them keep doing it.
>> +   xterms and use vi in all of them.  They also examine diffs (and three-way
>> +   diffs) on an 80-column terminal, accounting for two extra characters.
>> +   The best way to punish them is to let them keep doing it.
>>   - Code and especially patches is much more readable if limited to a sane
>>     line length.  Eighty is traditional.
>> + - The four-space indentation makes the most common excuse ("But look
>> +   at all that white space on the left!") moot.
>>   - It is the QEMU coding style.
>>  
>>  3. Naming
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 7f0aae9..bc32d8f 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1466,14 +1466,23 @@ sub process {
>>  # check we are in a valid source file if not then ignore this hunk
>>  		next if ($realfile !~ /\.(h|c|cpp|s|S|pl|sh)$/);
>>  
>> -#80 column limit
>> -		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
>> -		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
>> +#90 column limit
>> +		if ($line =~ /^\+/ &&
>>  		    !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ ||
>> -		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
>> -		    $length > 80)
>> +		    !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/))
>>  		{
>> -			WARN("line over 80 characters\n" . $herecurr);
>> +			if ($length > 90) {
>> +				ERROR("line over 90 characters\n" . $herecurr);
>> +			} elsif ($length > 76 && !($rawline =~ /^\+ \* /)) {
>> +				# The BSD license blurb has 80 character lines.
>> +				# Avoid warning on cut-and-pasted license text.
>> +				WARN("line over 76 characters\n" . $herecurr);
> 
> Sorry, this is _ugly_! First, the limit in QEMU has always been 80
> columns in the past, thus we've got tons of code that is written with 80
> columns already, so you suddenly even can not even copy-n-paste / move
> around code anymore that was valid before?!? That'll be a PITA since you
> have to reformat all patches that move code around - and it will also be
> a PITA for all reviewers since checking whether old code matches new
> code becomes more difficult.

QEMU has 2816 files.

529 files have at least one line with 81+ columns.

1342 files have at least one 77-80 columns line that would now warn.

1474 files have no lines with 77+ columns.

Some classification of line lengths (with the "BSD exception"):

	Length			Count
	77-80			16358
	78-80			11853
	79-80			7852
	80			2623
	81+			6588

Some other interesting data:

- However, only 605 files have 5 or more lines with 77+ columns, so the
odds of warnings after copy-n-paste are pretty slim.

- Also, of the 77+ column lines, about 10% are in macros that are
aligned right with a backslash.

- 1195 files have at least one 78-80 columns that would now warn; 979
files have at least one 79-80 columns that would now warn; 644 files
have at least one 80 columns that would now warn.

- With the above patch, the "BSD exception" hits 5471 times, of which
about 1900 are false positives.  If the warning is restricted to 80+
columns, the "BSD exception" is still necessary, but it only hits 1033
times and only 152 are false positives.

Based on the above data, I suggest having 79 columns as the limit (with
the "BSD exception" unfortunately).  This still makes it possible to
watch diffs on an 80-column terminal without wrapping.

Alternatively you would have 80 columns with no "BSD exception", which
is (more or less) what was in the previous submission of the patch.

> Second, I really don't like the idea of introducing exceptions here like
> it is done with the BSD license check above ... that will only cause
> confusion and "let's add another exception" situations later, I think.

I agree it's not nice.

> Can't we simply keep the 80 columns limit instead, please? IMHO people
> having trouble to view patches with 80 columns should simply be forced
> to switch to proper tools instead...

It's just the default size of a terminal.  If I do "<Ctrl-N> git show
<Right click> <Enter>" I get an 80-column terminal.

Paolo

ps: to find "BSD exception" false positives I used the following regex:
copy|deal|rights|included in|EXPRESS OR|OR OTHER|ARISING FROM,|at
your|or later.|by the Free|software without|THE USE OF
THIS|licenses/>.|its contributors|detailed
below.|USA.|USA|PURPOSE|CONSEQUENTIAL|STRICT

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Thomas Huth]

On 07/09/15 16:22, Paolo Bonzini wrote:
> 
> On 07/09/2015 15:18, Thomas Huth wrote:
>> On 07/09/15 11:53, Paolo Bonzini wrote:
>>> Line lengths above 80 characters do exist.  They are rare, but
>>> they happen from time to time.  An ignored rule is worse than an
>>> exception to the rule, so do the latter.
>>>
>>> Based on remarks from the list, make the preferred line length
>>> slightly lower than 80 characters, to account for extra characters
>>> in unified diffs (including three-way diffs) and for email quoting.
>>>
>>> Checkpatch has some code to detect doc comments that doesn't apply
>>> to QEMU; the usual limits apply even for doc comments in our case.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>  CODING_STYLE          | 13 ++++++++++---
>>>  scripts/checkpatch.pl | 21 +++++++++++++++------
>>>  2 files changed, 25 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/CODING_STYLE b/CODING_STYLE
>>> index 3c6978f..34d5526 100644
>>> --- a/CODING_STYLE
>>> +++ b/CODING_STYLE
>>> @@ -31,14 +31,21 @@ Do not leave whitespace dangling off the ends of lines.
>>>  
>>>  2. Line width
>>>  
>>> -Lines are 80 characters; not longer.
>>> +Lines should be 76 characters; try not to make them longer.
[...]
>> Sorry, this is _ugly_! First, the limit in QEMU has always been 80
>> columns in the past, thus we've got tons of code that is written with 80
>> columns already, so you suddenly even can not even copy-n-paste / move
>> around code anymore that was valid before?!? That'll be a PITA since you
>> have to reformat all patches that move code around - and it will also be
>> a PITA for all reviewers since checking whether old code matches new
>> code becomes more difficult.
[...]
> Some other interesting data:
> 
> - However, only 605 files have 5 or more lines with 77+ columns, so the
> odds of warnings after copy-n-paste are pretty slim.

Apart from copy-n-pasting, there is also the problem that you can run
"checkpatch.pl -f" on a whole file ... it would also be ugly to suddenly
have (much) more warnings here.

> Based on the above data, I suggest having 79 columns as the limit (with
> the "BSD exception" unfortunately).  This still makes it possible to
> watch diffs on an 80-column terminal without wrapping.

79 sounds at least better to me than 76. So if you really want to change
the limit, please go for 79 instead of 76.

 Thomas

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Line lengths above 80 characters do exist.  They are rare, but
> they happen from time to time.  An ignored rule is worse than an
> exception to the rule, so do the latter.
>
> Based on remarks from the list, make the preferred line length
> slightly lower than 80 characters, to account for extra characters
> in unified diffs (including three-way diffs) and for email quoting.
>
> Checkpatch has some code to detect doc comments that doesn't apply
> to QEMU; the usual limits apply even for doc comments in our case.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  CODING_STYLE          | 13 ++++++++++---
>  scripts/checkpatch.pl | 21 +++++++++++++++------
>  2 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index 3c6978f..34d5526 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -31,14 +31,21 @@ Do not leave whitespace dangling off the ends of lines.
>  
>  2. Line width
>  
> -Lines are 80 characters; not longer.
> +Lines should be 76 characters; try not to make them longer.
> +
> +Sometimes it is hard to do, especially when dealing with QEMU subsystems
> +that use long function or symbol names.  Even in that case, do not make
> +lines much longer than 76 characters.
>  
>  Rationale:
>   - Some people like to tile their 24" screens with a 6x4 matrix of 80x24
> -   xterms and use vi in all of them.  The best way to punish them is to
> -   let them keep doing it.
> +   xterms and use vi in all of them.  They also examine diffs (and three-way
> +   diffs) on an 80-column terminal, accounting for two extra characters.
> +   The best way to punish them is to let them keep doing it.
>   - Code and especially patches is much more readable if limited to a sane
>     line length.  Eighty is traditional.
> + - The four-space indentation makes the most common excuse ("But look
> +   at all that white space on the left!") moot.
>   - It is the QEMU coding style.
>  
>  3. Naming
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7f0aae9..bc32d8f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1466,14 +1466,23 @@ sub process {
>  # check we are in a valid source file if not then ignore this hunk
>  		next if ($realfile !~ /\.(h|c|cpp|s|S|pl|sh)$/);
>  
> -#80 column limit
> -		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
> -		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
> +#90 column limit
> +		if ($line =~ /^\+/ &&
>  		    !($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ ||
> -		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
> -		    $length > 80)
> +		    !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/))
>  		{
> -			WARN("line over 80 characters\n" . $herecurr);
> +			if ($length > 90) {
> +				ERROR("line over 90 characters\n" . $herecurr);
> +			} elsif ($length > 76 && !($rawline =~ /^\+ \* /)) {
> +				# The BSD license blurb has 80 character lines.
> +				# Avoid warning on cut-and-pasted license text.

Why not simply reflow all the offending license blurbs?

Want me to prep such a patch?

> +				WARN("line over 76 characters\n" . $herecurr);
> +			} elsif ($length > 80) {
> +				# Do not confuse the user by introducing yet
> +				# another limit (80 characters).  Technically,
> +				# this line *is* over 76 characters.
> +				WARN("line over 76 characters\n" . $herecurr);
> +			}
>  		}
>  
>  # check for spaces before a quoted newline

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 07/09/15 16:22, Paolo Bonzini wrote:
>> 
>> On 07/09/2015 15:18, Thomas Huth wrote:
>>> On 07/09/15 11:53, Paolo Bonzini wrote:
>>>> Line lengths above 80 characters do exist.  They are rare, but
>>>> they happen from time to time.  An ignored rule is worse than an
>>>> exception to the rule, so do the latter.
>>>>
>>>> Based on remarks from the list, make the preferred line length
>>>> slightly lower than 80 characters, to account for extra characters
>>>> in unified diffs (including three-way diffs) and for email quoting.
>>>>
>>>> Checkpatch has some code to detect doc comments that doesn't apply
>>>> to QEMU; the usual limits apply even for doc comments in our case.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>  CODING_STYLE          | 13 ++++++++++---
>>>>  scripts/checkpatch.pl | 21 +++++++++++++++------
>>>>  2 files changed, 25 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/CODING_STYLE b/CODING_STYLE
>>>> index 3c6978f..34d5526 100644
>>>> --- a/CODING_STYLE
>>>> +++ b/CODING_STYLE
>>>> @@ -31,14 +31,21 @@ Do not leave whitespace dangling off the ends of lines.
>>>>  
>>>>  2. Line width
>>>>  
>>>> -Lines are 80 characters; not longer.
>>>> +Lines should be 76 characters; try not to make them longer.
> [...]
>>> Sorry, this is _ugly_! First, the limit in QEMU has always been 80
>>> columns in the past, thus we've got tons of code that is written with 80
>>> columns already, so you suddenly even can not even copy-n-paste / move
>>> around code anymore that was valid before?!? That'll be a PITA since you
>>> have to reformat all patches that move code around - and it will also be
>>> a PITA for all reviewers since checking whether old code matches new
>>> code becomes more difficult.

Feature.  Nudging developers to tidy up style when they touch code is
what we've always done.  You're not entitled to be able to copy existing
code unchanged.

Now from a reviewer's point of view: I can't recall a review where style
cleanup caused me trouble, *except* when mixed up with other changes.
But that's an orthogonal no-no.

> [...]
>> Some other interesting data:
>> 
>> - However, only 605 files have 5 or more lines with 77+ columns, so the
>> odds of warnings after copy-n-paste are pretty slim.
>
> Apart from copy-n-pasting, there is also the problem that you can run
> "checkpatch.pl -f" on a whole file ... it would also be ugly to suddenly
> have (much) more warnings here.

Feature.  If you run checkpatch on a whole file, you obviously do it to
find its ugly spots.  Lines longer than 76 characters qualify.

>> Based on the above data, I suggest having 79 columns as the limit (with
>> the "BSD exception" unfortunately).  This still makes it possible to
>> watch diffs on an 80-column terminal without wrapping.
>
> 79 sounds at least better to me than 76. So if you really want to change
> the limit, please go for 79 instead of 76.

Have you read the thread leading up to this patch?

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Paolo Bonzini]

On 07/09/2015 17:17, Markus Armbruster wrote:
>> > +			} elsif ($length > 76 && !($rawline =~ /^\+ \* /)) {
>> > +				# The BSD license blurb has 80 character lines.
>> > +				# Avoid warning on cut-and-pasted license text.
> Why not simply reflow all the offending license blurbs?
> 
> Want me to prep such a patch?

Actually there are other license blurbs with 80-character lines.  It's
just that for the GPL we just refer to the COPYING file most of the
time, instead of TYPING SEVERAL LINES ABOUT MERCHANTABILITY, whatever it is.

Paolo

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Paolo Bonzini]

On 07/09/2015 17:23, Markus Armbruster wrote:
> > Apart from copy-n-pasting, there is also the problem that you can run
> > "checkpatch.pl -f" on a whole file ... it would also be ugly to suddenly
> > have (much) more warnings here.
> 
> Feature.  If you run checkpatch on a whole file, you obviously do it to
> find its ugly spots.  Lines longer than 76 characters qualify.

Based on the statistics, half of QEMU's files has at least one 76-79
character line.  The noise from checkpatch.pl -f is actually a worse
thing than the cut-and-paste, but that's something that can be fixed in
other ways (e.g. different strictness for checkpatch.pl vs.
checkpatch.pl -f).

That said, and even though Thomas obviously hasn't read the previous
discussion, :) I do believe that 76 characters is too strict a limit.

76 would be great (two levels of email quoting are what you get 99% of
the time), and 78 would be nice, but I believe 79 provides the biggest
bang for the buck.

Paolo

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 07/09/2015 17:17, Markus Armbruster wrote:
>>> > +			} elsif ($length > 76 && !($rawline =~ /^\+ \* /)) {
>>> > +				# The BSD license blurb has 80 character lines.
>>> > +				# Avoid warning on cut-and-pasted license text.
>> Why not simply reflow all the offending license blurbs?
>> 
>> Want me to prep such a patch?
>
> Actually there are other license blurbs with 80-character lines.  It's
> just that for the GPL we just refer to the COPYING file most of the
> time, instead of TYPING SEVERAL LINES ABOUT MERCHANTABILITY, whatever it is.

My offer to "reflow all the offending license blurbs" includes all of
them.

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Thomas Huth]

On 07/09/15 17:23, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 07/09/15 16:22, Paolo Bonzini wrote:
>> [...]
>>> Some other interesting data:
>>>
>>> - However, only 605 files have 5 or more lines with 77+ columns, so the
>>> odds of warnings after copy-n-paste are pretty slim.
>>
>> Apart from copy-n-pasting, there is also the problem that you can run
>> "checkpatch.pl -f" on a whole file ... it would also be ugly to suddenly
>> have (much) more warnings here.
> 
> Feature.  If you run checkpatch on a whole file, you obviously do it to
> find its ugly spots.  Lines longer than 76 characters qualify.

I sometimes also use "checkpatch.pl -f" to check my own new code that
I've added to files (when I did not create patch files yet). When it
suddenly spams me because of such new line-length warnings, that would
be quite annoying, I think.

>>> Based on the above data, I suggest having 79 columns as the limit (with
>>> the "BSD exception" unfortunately).  This still makes it possible to
>>> watch diffs on an 80-column terminal without wrapping.
>>
>> 79 sounds at least better to me than 76. So if you really want to change
>> the limit, please go for 79 instead of 76.
> 
> Have you read the thread leading up to this patch?

You mean the v1 thread of this patch? I slightly recall it ... and just
looked it up again. As far as I can see, you were the only one who
suggested such a small limit, John wanted to keep it at 80 columns and
Andreas suggested 78.

I can see your point with "typographic style suggests 60 characters",
but I think you can not really fully apply this to source code - it
reads much different that natural text, e.g. there are much more
separating special characters like parentheses and commas in between.

We could maybe mention that 76 columns limit in the coding style
document, to make people aware that they should start thinking about
breaking lines in two when they reach 76 characters in a line. But
changing checkpatch.pl to use this limit really sounds too harsh to me.
Maybe 79, yes, since that really gives to the possibility to nicely view
patches in a 80 columns terminal, too. (But if you try to view a 2-way
patch in a 80-columns window, you quite masochistic anyway, so I don't
see the point in reducing it to 78). Less than 79 would IMHO cause too
much noise with the already existing code, so I really would appreciate
if checkpatch.pl would not use less than that.

 Thomas

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 07/09/2015 17:23, Markus Armbruster wrote:
>> > Apart from copy-n-pasting, there is also the problem that you can run
>> > "checkpatch.pl -f" on a whole file ... it would also be ugly to suddenly
>> > have (much) more warnings here.
>> 
>> Feature.  If you run checkpatch on a whole file, you obviously do it to
>> find its ugly spots.  Lines longer than 76 characters qualify.
>
> Based on the statistics, half of QEMU's files has at least one 76-79
> character line.  The noise from checkpatch.pl -f is actually a worse
> thing than the cut-and-paste, but that's something that can be fixed in
> other ways (e.g. different strictness for checkpatch.pl vs.
> checkpatch.pl -f).

Yup.

> That said, and even though Thomas obviously hasn't read the previous
> discussion, :) I do believe that 76 characters is too strict a limit.

It's not a strict limit, it's a warning.  The strict limit is 90.

> 76 would be great (two levels of email quoting are what you get 99% of
> the time), and 78 would be nice, but I believe 79 provides the biggest
> bang for the buck.

78 gives one level of quoting, and two-way diffs.

Re: [Qemu-devel] [PATCH 1/4] CODING_STYLE: update mixed declaration rules

[Stefan Hajnoczi]

On Mon, Sep 07, 2015 at 11:53:01AM +0200, Paolo Bonzini wrote:
> Mixed declarations also do exist at the top of #ifdef blocks.
> Reluctantly allow this particular usage and suggest an alternative.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  CODING_STYLE | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Stefan Hajnoczi]

On Mon, Sep 07, 2015 at 11:53:03AM +0200, Paolo Bonzini wrote:
> Mostly change severity levels, but some tests can also be adjusted to refer
> to QEMU APIs or data structures.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/checkpatch.pl | 141 +++++++++++++++++++++-----------------------------
>  1 file changed, 60 insertions(+), 81 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH 1/4] CODING_STYLE: update mixed declaration rules

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Mixed declarations also do exist at the top of #ifdef blocks.
> Reluctantly allow this particular usage and suggest an alternative.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  CODING_STYLE | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index d46cfa5..3c6978f 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -87,10 +87,15 @@ Furthermore, it is the QEMU coding style.
>  
>  5. Declarations
>  
> -Mixed declarations (interleaving statements and declarations within blocks)
> -are not allowed; declarations should be at the beginning of blocks.  In other
> -words, the code should not generate warnings if using GCC's
> --Wdeclaration-after-statement option.
> +Mixed declarations (interleaving statements and declarations within
> +blocks) are generally not allowed; declarations should be at the beginning
> +of blocks.
> +
> +Every now and then, an exception is made for declarations inside a
> +#ifdef or #ifndef block: if the code looks nicer, such declarations can
> +be placed at the top of the block even if there are statements above.
> +On the other hand, however, it's often best to move that #ifdef/#ifndef
> +block to a separate function altogether.
>  
>  6. Conditional statements

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[John Snow]

On 09/07/2015 01:05 PM, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 07/09/2015 17:23, Markus Armbruster wrote:
>>>> Apart from copy-n-pasting, there is also the problem that you can run
>>>> "checkpatch.pl -f" on a whole file ... it would also be ugly to suddenly
>>>> have (much) more warnings here.
>>>
>>> Feature.  If you run checkpatch on a whole file, you obviously do it to
>>> find its ugly spots.  Lines longer than 76 characters qualify.
>>
>> Based on the statistics, half of QEMU's files has at least one 76-79
>> character line.  The noise from checkpatch.pl -f is actually a worse
>> thing than the cut-and-paste, but that's something that can be fixed in
>> other ways (e.g. different strictness for checkpatch.pl vs.
>> checkpatch.pl -f).
> 
> Yup.
> 
>> That said, and even though Thomas obviously hasn't read the previous
>> discussion, :) I do believe that 76 characters is too strict a limit.
> 
> It's not a strict limit, it's a warning.  The strict limit is 90.
> 

The problem is that checkpatch returns non-zero for warnings, so this
interrupts my workflow; this means that many of my "patch testing"
scripts will now "fail" due to the shortened limit.

Unless there are documented error codes for checkpatch -- e.g. {0: fine,
1: warnings, 2: errors, 3+: script-errors }

I could then update my scripts to tolerate warnings, but this still
seems like a pain to me.

>> 76 would be great (two levels of email quoting are what you get 99% of
>> the time), and 78 would be nice, but I believe 79 provides the biggest
>> bang for the buck.
> 
> 78 gives one level of quoting, and two-way diffs.
> 

-- 
—js

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Peter Maydell]

On 7 September 2015 at 18:05, Markus Armbruster <armbru@redhat.com> wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> That said, and even though Thomas obviously hasn't read the previous
>> discussion, :) I do believe that 76 characters is too strict a limit.
>
> It's not a strict limit, it's a warning.  The strict limit is 90.

I tend to bounce patches on review for checkpatch warnings...
I don't make much distinction between a warning and an error.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Eduardo Habkost]

On Mon, Sep 07, 2015 at 07:02:55PM +0200, Thomas Huth wrote:
[...]
> We could maybe mention that 76 columns limit in the coding style
> document, to make people aware that they should start thinking about
> breaking lines in two when they reach 76 characters in a line. But
> changing checkpatch.pl to use this limit really sounds too harsh to me.

Please don't. The whole point of checkpatch.pl is to let a robot do the
work of checking coding style instead of relying on us humans.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Eduardo Habkost]

On Mon, Sep 07, 2015 at 06:06:25PM +0200, Paolo Bonzini wrote:
> On 07/09/2015 17:23, Markus Armbruster wrote:
> > > Apart from copy-n-pasting, there is also the problem that you can run
> > > "checkpatch.pl -f" on a whole file ... it would also be ugly to suddenly
> > > have (much) more warnings here.
> > 
> > Feature.  If you run checkpatch on a whole file, you obviously do it to
> > find its ugly spots.  Lines longer than 76 characters qualify.
> 
> Based on the statistics, half of QEMU's files has at least one 76-79
> character line.  The noise from checkpatch.pl -f is actually a worse
> thing than the cut-and-paste, but that's something that can be fixed in
> other ways (e.g. different strictness for checkpatch.pl vs.
> checkpatch.pl -f).

Why exactly it's considered noise? I fail to see why would somebody use
checkpatch.pl -f if they don't want to see all warnings about the whole
file (including the lines that they didn't write). If somebody doesn't
want to see them, they can simply run checkpatch.pl on the diff instead
of using -f.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Eduardo Habkost]

On Wed, Sep 09, 2015 at 06:22:15PM +0100, Peter Maydell wrote:
> On 7 September 2015 at 18:05, Markus Armbruster <armbru@redhat.com> wrote:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> >> That said, and even though Thomas obviously hasn't read the previous
> >> discussion, :) I do believe that 76 characters is too strict a limit.
> >
> > It's not a strict limit, it's a warning.  The strict limit is 90.
> 
> I tend to bounce patches on review for checkpatch warnings...
> I don't make much distinction between a warning and an error.

If there are existing warnings that will make you bounce patches
automatically, shouldn't they become errors?

I believe there are things we want to warn people about on checkpatch.pl
because they should be discouraged (like lines a bit longer than
76^H^H78^H^H79 columns), but that shouldn't make a patch be
automatically rejected.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Paolo Bonzini]

On 09/09/2015 19:22, Peter Maydell wrote:
> On 7 September 2015 at 18:05, Markus Armbruster <armbru@redhat.com> wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> That said, and even though Thomas obviously hasn't read the previous
>>> discussion, :) I do believe that 76 characters is too strict a limit.
>>
>> It's not a strict limit, it's a warning.  The strict limit is 90.
> 
> I tend to bounce patches on review for checkpatch warnings...
> I don't make much distinction between a warning and an error.

Doing something about this is the point of this series.  As a start, it
enables a few more warnings (patch 3) so that maintainers start seeing
them and get used to them.  Some may reject patches altogether based on
the warnings, some may not.

With just a few exceptions (tabs, line lengths, braces) it makes a lot
of sense to fix warnings in the whole tree with a single sweeping
change.  If we agree about this, we can upgrade warnings to error at the
time of the fix, or even before.

Ultimately, warnings should be used only for things that are _really_
subjective, so that we can aim at an error-free (but not warning-free)
"checkpatch -f".

Paolo

Re: [Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Peter Maydell]

On 7 September 2015 at 10:53, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Mostly change severity levels, but some tests can also be adjusted to refer
> to QEMU APIs or data structures.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> @@ -1956,9 +1941,14 @@ sub process {
>                         ERROR("open brace '{' following $1 go on the same line\n" . $hereprev);
>                 }
>
> +# ... however, open braces on typedef lines should be avoided.
> +               if ($line =~ /^.\s*typedef\s+(enum|union|struct)(?:\s+$Ident\b)?.*[^;]$/) {
> +                       ERROR("typedefs should be separate from struct declaration\n" . $herecurr);
> +               }
> +
>  # missing space after union, struct or enum definition

Can we revert this one, please? Checkpatch now warns about constructs
like
  typedef struct MyDevice {
      DeviceState parent;

      int reg0, reg1, reg2;
  } MyDevice;

which I think are common practice throughout QEMU (recommended
as part of the examples in include/qom/object.h and
in http://wiki.qemu.org/QOMConventions, for instance).

$ git grep 'typedef struct.*{' |wc -l
1884

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Paolo Bonzini]

On 17/09/2015 16:24, Peter Maydell wrote:
> Can we revert this one, please? Checkpatch now warns about constructs
> like
>   typedef struct MyDevice {
>       DeviceState parent;
> 
>       int reg0, reg1, reg2;
>   } MyDevice;

It's interesting that qom/object.h documents this and start like:

typedef struct ObjectClass ObjectClass;
typedef struct Object Object;

typedef struct TypeInfo TypeInfo;

typedef struct InterfaceClass InterfaceClass;
typedef struct InterfaceInfo InterfaceInfo;

I have a patch to flag widely-disrespected rules that we still want to
encourage in patches.  Would you agree with filing these typedefs under
this category?

Paolo

Re: [Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Peter Maydell]

On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 17/09/2015 16:24, Peter Maydell wrote:
>> Can we revert this one, please? Checkpatch now warns about constructs
>> like
>>   typedef struct MyDevice {
>>       DeviceState parent;
>>
>>       int reg0, reg1, reg2;
>>   } MyDevice;
>
> It's interesting that qom/object.h documents this and start like:
>
> typedef struct ObjectClass ObjectClass;
> typedef struct Object Object;
>
> typedef struct TypeInfo TypeInfo;
>
> typedef struct InterfaceClass InterfaceClass;
> typedef struct InterfaceInfo InterfaceInfo;
>
> I have a patch to flag widely-disrespected rules that we still want to
> encourage in patches.  Would you agree with filing these typedefs under
> this category?

No, I think that having a separate typedef is worse. The
only exceptions are (a) when you need it to be separate because
you need to use the type within the struct itself (or some
similar dependency loop) (b) when you want to put the typedef
in include/qemu/typedefs.h.

I really don't see any need to suddenly outlaw something
that's been accepted as standard good QEMU style for a
long time.

(You could make checkpatch warn about
  typedef struct Foo
  {
      stuff;
  } Foo;

if you like, the newline before the '{' is out of line with our
usual approach.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Paolo Bonzini]

On 17/09/2015 18:16, Peter Maydell wrote:
> On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 17/09/2015 16:24, Peter Maydell wrote:
>>> Can we revert this one, please? Checkpatch now warns about constructs
>>> like
>>>   typedef struct MyDevice {
>>>       DeviceState parent;
>>>
>>>       int reg0, reg1, reg2;
>>>   } MyDevice;
>>
>> It's interesting that qom/object.h documents this and start like:
>>
>> typedef struct ObjectClass ObjectClass;
>> typedef struct Object Object;
>>
>> typedef struct TypeInfo TypeInfo;
>>
>> typedef struct InterfaceClass InterfaceClass;
>> typedef struct InterfaceInfo InterfaceInfo;
>>
>> I have a patch to flag widely-disrespected rules that we still want to
>> encourage in patches.  Would you agree with filing these typedefs under
>> this category?
> 
> No, I think that having a separate typedef is worse. The
> only exceptions are (a) when you need it to be separate because
> you need to use the type within the struct itself (or some
> similar dependency loop) (b) when you want to put the typedef
> in include/qemu/typedefs.h.
> 
> I really don't see any need to suddenly outlaw something
> that's been accepted as standard good QEMU style for a
> long time.

I think it varies depending on the maintainer.  PPC, USB, SCSI, ACPI all
use a separate typedef.  I'll prepare a revert.

Paolo

Re: [Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

On 09/17/2015 10:32 AM, Paolo Bonzini wrote:

>>>> Can we revert this one, please? Checkpatch now warns about constructs
>>>> like
>>>>   typedef struct MyDevice {
>>>>       DeviceState parent;
>>>>
>>>>       int reg0, reg1, reg2;
>>>>   } MyDevice;
>>>
>>> It's interesting that qom/object.h documents this and start like:
>>>
>>> typedef struct ObjectClass ObjectClass;

> I think it varies depending on the maintainer.  PPC, USB, SCSI, ACPI all
> use a separate typedef.  I'll prepare a revert.

And qapi is about to switch from inline to separate:

https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04291.html

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/17/2015 10:32 AM, Paolo Bonzini wrote:
>
>>>>> Can we revert this one, please? Checkpatch now warns about constructs
>>>>> like
>>>>>   typedef struct MyDevice {
>>>>>       DeviceState parent;
>>>>>
>>>>>       int reg0, reg1, reg2;
>>>>>   } MyDevice;
>>>>
>>>> It's interesting that qom/object.h documents this and start like:
>>>>
>>>> typedef struct ObjectClass ObjectClass;
>
>> I think it varies depending on the maintainer.  PPC, USB, SCSI, ACPI all
>> use a separate typedef.  I'll prepare a revert.
>
> And qapi is about to switch from inline to separate:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04291.html

For a technical reason: DRY in the generator source code.

For some types, we need the typedef name declared in one place, and the
struct defined in another.  Instead of having two variants, one for
separate typedef / struct and one for combined, plus the logic to decide
which one to use, we simply generate separate always.

Re: [Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Peter Maydell]

On 17 September 2015 at 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 17/09/2015 18:16, Peter Maydell wrote:
>> On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>
>>> On 17/09/2015 16:24, Peter Maydell wrote:
>>>> Can we revert this one, please? Checkpatch now warns about constructs
>>>> like
>>>>   typedef struct MyDevice {
>>>>       DeviceState parent;
>>>>
>>>>       int reg0, reg1, reg2;
>>>>   } MyDevice;

> I think it varies depending on the maintainer.  PPC, USB, SCSI, ACPI all
> use a separate typedef.  I'll prepare a revert.

Ping on that revert patch? I can't find it onlist...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Eduardo Habkost]

On Sun, Oct 04, 2015 at 07:44:29PM +0100, Peter Maydell wrote:
> On 17 September 2015 at 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >
> > On 17/09/2015 18:16, Peter Maydell wrote:
> >> On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>>
> >>>
> >>> On 17/09/2015 16:24, Peter Maydell wrote:
> >>>> Can we revert this one, please? Checkpatch now warns about constructs
> >>>> like
> >>>>   typedef struct MyDevice {
> >>>>       DeviceState parent;
> >>>>
> >>>>       int reg0, reg1, reg2;
> >>>>   } MyDevice;
> 
> > I think it varies depending on the maintainer.  PPC, USB, SCSI, ACPI all
> > use a separate typedef.  I'll prepare a revert.
> 
> Ping on that revert patch? I can't find it onlist...

The x86 pull request I sent today triggers this error, BTW. I hope it
won't make the pull request be automatically rejected.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Peter Maydell]

On 5 October 2015 at 19:11, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Sun, Oct 04, 2015 at 07:44:29PM +0100, Peter Maydell wrote:
>> On 17 September 2015 at 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >
>> >
>> > On 17/09/2015 18:16, Peter Maydell wrote:
>> >> On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >>>
>> >>>
>> >>> On 17/09/2015 16:24, Peter Maydell wrote:
>> >>>> Can we revert this one, please? Checkpatch now warns about constructs
>> >>>> like
>> >>>>   typedef struct MyDevice {
>> >>>>       DeviceState parent;
>> >>>>
>> >>>>       int reg0, reg1, reg2;
>> >>>>   } MyDevice;
>>
>> > I think it varies depending on the maintainer.  PPC, USB, SCSI, ACPI all
>> > use a separate typedef.  I'll prepare a revert.
>>
>> Ping on that revert patch? I can't find it onlist...
>
> The x86 pull request I sent today triggers this error, BTW. I hope it
> won't make the pull request be automatically rejected.

Nope, because I don't run checkpatch on pull requests.

-- PMM

Re: [Qemu-devel] [PATCH 3/4] checkpatch: adapt some tests to QEMU

[Paolo Bonzini]

On 05/10/2015 20:47, Peter Maydell wrote:
> On 5 October 2015 at 19:11, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> On Sun, Oct 04, 2015 at 07:44:29PM +0100, Peter Maydell wrote:
>>> On 17 September 2015 at 17:32, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>>
>>>> On 17/09/2015 18:16, Peter Maydell wrote:
>>>>> On 17 September 2015 at 17:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 17/09/2015 16:24, Peter Maydell wrote:
>>>>>>> Can we revert this one, please? Checkpatch now warns about constructs
>>>>>>> like
>>>>>>>   typedef struct MyDevice {
>>>>>>>       DeviceState parent;
>>>>>>>
>>>>>>>       int reg0, reg1, reg2;
>>>>>>>   } MyDevice;
>>>
>>>> I think it varies depending on the maintainer.  PPC, USB, SCSI, ACPI all
>>>> use a separate typedef.  I'll prepare a revert.
>>>
>>> Ping on that revert patch? I can't find it onlist...
>>
>> The x86 pull request I sent today triggers this error, BTW. I hope it
>> won't make the pull request be automatically rejected.
> 
> Nope, because I don't run checkpatch on pull requests.

I've sent the patch today, by the way.  Sorry for the delay.

Paolo