[PATCH 9/8] lib/sort: turn off self-test

[PATCH 9/8] lib/sort: turn off self-test

[Matt Mackall]

Doh.

Signed-off-by: Matt Mackall <mpm@selenic.com>

Index: mm2/lib/sort.c
===================================================================
--- mm2.orig/lib/sort.c	2005-01-30 22:37:28.000000000 -0800
+++ mm2/lib/sort.c	2005-01-30 23:41:40.000000000 -0800
@@ -82,7 +82,7 @@
 
 MODULE_LICENSE("GPL");
 
-#if 1
+#if 0
 /* a simple boot-time regression test */
 
 int cmpint(const void *a, const void *b)


-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 9/8] lib/sort: turn off self-test

[Paul Jackson]

How about just removing the self test, not "#if 0"'ing it out.

Better to keep the kernel source code clean of development
scaffolding.

Though your patch 1/8 hasn't arrived in my email inbox yet,
so I don't actually know what 'self test' code it is that
I am speaking of ;).

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.650.933.1373, 1.925.600.0401

Re: [PATCH 9/8] lib/sort: turn off self-test

[Andreas Gruenbacher]

On Mon, 2005-01-31 at 12:57, Paul Jackson wrote:
> How about just removing the self test, not "#if 0"'ing it out.

I agree with that.

-- Andreas.

Re: [PATCH 9/8] lib/sort: turn off self-test

[Matt Mackall]

On Mon, Jan 31, 2005 at 03:57:42AM -0800, Paul Jackson wrote:
> How about just removing the self test, not "#if 0"'ing it out.
> 
> Better to keep the kernel source code clean of development
> scaffolding.
> 
> Though your patch 1/8 hasn't arrived in my email inbox yet,
> so I don't actually know what 'self test' code it is that
> I am speaking of ;).

It's a nice self-contained unit test. It's here because I ran into a
strange regparm-related bug when developing the code in userspace and
I wanted to be sure that it was easy to diagnose in the field if a
similar bug appeared in the future. I actually think that more code
ought to have such tests, so long as they don't obscure the code in
question.

Here it is for purposes of discussion:

+#if 1
+/* a simple boot-time regression test */
+
+int cmpint(const void *a, const void *b)
+{
+	return *(int *)a - *(int *)b;
+}
+
+static int sort_test(void)
+{
+	int *a, i, r = 0;
+
+	a = kmalloc(1000 * sizeof(int), GFP_KERNEL);
+	BUG_ON(!a);
+
+	printk("testing sort()\n");
+
+	for (i = 0; i < 1000; i++) {
+		r = (r * 725861) % 6599;
+		a[i] = r;
+	}
+
+	sort(a, 1000, sizeof(int), cmpint, 0);
+
+	for (i = 0; i < 999; i++)
+		if (a[i] > a[i+1]) {
+			printk("sort() failed!\n");
+			break;
+		}
+
+	kfree(a);
+
+	return 0;
+}
+
+module_init(sort_test);
+#endif

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 9/8] lib/sort: turn off self-test

[Paul Jackson]

Matt wrote:
> I actually think that more code
> ought to have such tests, so long as they don't obscure the code in
> question.

If all the little unit tests that had ever been coded for kernel code
were embedded in "#if 0 ... #endif" sections, it _would_ obscure the
code.

To the local maintainer of a file, it's not much of an issue, as they
are actively familiar with what's there and look past the disabled test.
But to the stranger passing through, it's just another three dozen lines
of code that need to be waded through, or another false hit or two on
some wide ranging grep that needs to be filtered out.

Instead, I'd suggest a couple of comments:

 1) Describe this unit test in a comment, such as:

	/*
	 * Nice unit test for this sort: try sorting the array
	 * of 1000 integers constructed with the loop:
	 *	for (i = 0; i < 1000; i ++)
	 *		a[i] = r = (r * 725861) % 6599;
	 * Sort then verify that for each i: a[i] <= a[i+1].
	 */

    Five lines of comment are substantially less intrusive
    than three dozen lines of code.  Both smaller, and even
    in the output of a grep, visually distinct.

 2) Describe whatever it was you stumbled on that's behind
    your comment:

	I ran into a strange regparm-related bug ...

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.650.933.1373, 1.925.600.0401

Re: [PATCH 9/8] lib/sort: turn off self-test

[Werner Almesberger]

Matt Mackall wrote:
> It's a nice self-contained unit test.

By the way, I think it would be useful if there was a more
formalized frame for such unit tests, so that they could be used
in automated kernel testing.

To avoid false positives when grepping through the code, perhaps
such tests could be placed in separate files, using a different
extension, e.g. .ct for ".c test", or in some "test" directory.
(That is, in case they're distributed along with the kernel code,
which, IMHO, would help to avoid code drift.)

Just an idea ...

- Werner

-- 
  _________________________________________________________________________
 / Werner Almesberger, Buenos Aires, Argentina         wa@almesberger.net /
/_http://www.almesberger.net/____________________________________________/

Re: [PATCH 9/8] lib/sort: turn off self-test

[Pekka Enberg]

On Mon, 31 Jan 2005 09:03:44 -0800, Matt Mackall <mpm@selenic.com> wrote:
> It's a nice self-contained unit test. It's here because I ran into a
> strange regparm-related bug when developing the code in userspace and
> I wanted to be sure that it was easy to diagnose in the field if a
> similar bug appeared in the future. I actually think that more code
> ought to have such tests, so long as they don't obscure the code in
> question.

Unit tests are nice and your approach is wrong. The test does not
belong in the implementation for two reasons: it hurts readability of
the actual code and the _commented out_ test will not be maintained
(dead code never is).

I don't know if the maintainers are interested in unit tests but a
better solution would be to  put your test in a separate file and make
sure it is always compiled and executed when CONFIG_UNIT_TEST is
enabled.

P.S. If the test fails, it probably should do BUG().

                               Pekka