This is the mail archive of the
mailing list for the glibc project.
Re: [PATCH] vfprint: validate nargs and argument-based offsets
> + tst-vfprintf-nargs
We tend to call them bug-foo when they are regression tests for particular
bugs found in the past, and tst-foo when they are preemptively-written
tests. But there is no real consistency about that and nobody really cares.
This really warrants a bugzilla report. Then you should put:
at the top of the ChangeLog stanza. It's probably also worthwhile to
mention the bug reference inside the test case source.
Make a function static whenever it can be.
> +format_failed(const char *fmt, const char *expected, int wanted)
Space before paren (here and many other places).
> + /* Since the stack could be extremely wrecked by this test, use
> + an external supervisor process to catch the signals, since a
> + signal handler may not be able to recover.
> + */
*/ goes on the preceding line, after two spaces.
I'll give some more style comments below for future reference.
But this part doesn't actually belong here. Use the test-skeleton.c
> + if (pid)
We usually eschew implicit Boolean conversions, so write "if (pid != 0)"
(or > 0, here).
> + waitpid(pid, &status, 0);
Fails to check the return value.
> + fprintf(stderr, "Unexpected failure\n");
We generally use fputs rather than fprintf for constant strings.
> + memset(output, 0, sizeof(output));
We generally use "sizeof variable" rather than "sizeof (variable)".
The parens are only required when it's a type name.
> + /* Positional arguments are constructed via read_int(), so nargs
> + can only overflow on 32bit systems. On 64bit systems, it will
> + attempt to allocate a giant amount of stack memory and crash,
> + which is the expected situation. */
Two spaces after every sentence. Say "64-bit", not "64bit".
> + if (sizeof(long) == 4)
We eschew implicit "int", i.e. use "long int" instead of "long".
Contrary to what Ryan said, I think it's generally preferable to use if
rather than #if whenever you can. That avoids compile-time bit rot in the
code path that someone doesn't remember to update and doesn't always test.
> + if (format_failed(buf, "1 %$d", wanted)) rc = 1;
Never put the consequent on the same line.
> + /* Check for potential integer overflow. */
Still need two spaces after the sentence here, and below.