| Message ID | 58f2b0394546e8da2922adcbc38bdb6b53f2b313.1606545878.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Minimal patches to let reftable pass the CI builds | expand |
On Sat, Nov 28, 2020 at 06:44:35AM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 0-sized arrays are actually not portable. Definitely. > static void test_sizes_to_segments_empty(void) > { > - uint64_t sizes[0]; > + uint64_t sizes[1]; > > int seglen = 0; > struct segment *segs = > - sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes)); > + sizes_to_segments(&seglen, sizes, 0); > EXPECT(seglen == 0); > reftable_free(segs); I think passing: sizes_to_segments(&seglen, NULL, 0); may make the code more obvious. Unlike system functions like memcpy(), we can be assured of whether our functions avoid looking at a zero-length array (and size_to_segments does follow that rule). This function, of course, is nonsense that real code would not do, and is just unit-testing sizes_to_segments. I'm not wild in general about having a parallel suite of C tests that does not interact with our usual tests, but it may be the least bad way to benefit from the unit-test coverage that reftable ships with. As a rule, I'd much rather see a tool exposing functionality to the command-line, which can then be driven independently. I recognize that can end up complicated, though. -Peff
On Tue, Dec 1, 2020 at 11:26 AM Jeff King <peff@peff.net> wrote: > > On Sat, Nov 28, 2020 at 06:44:35AM +0000, Johannes Schindelin via GitGitGadget wrote: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > 0-sized arrays are actually not portable. > > Definitely. > > > static void test_sizes_to_segments_empty(void) > > { > > - uint64_t sizes[0]; > > + uint64_t sizes[1]; > > > > int seglen = 0; > > struct segment *segs = > > - sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes)); > > + sizes_to_segments(&seglen, sizes, 0); > > EXPECT(seglen == 0); > > reftable_free(segs); > > I think passing: > > sizes_to_segments(&seglen, NULL, 0); > > may make the code more obvious. Unlike system functions like memcpy(), > we can be assured of whether our functions avoid looking at a > zero-length array (and size_to_segments does follow that rule). > > This function, of course, is nonsense that real code would not do, and This test was added because 'real' code did this. In particular, if you initialize a stack of reftables, the stack has zero elements. The test was for the following bugfix https://github.com/google/reftable/commit/b2e42ecb54e413e494c1fcc13c21e24422645007 Changing the array size to 1 here also prevents Valgrind to mark a regression as a OOB write.
On Tue, Dec 01, 2020 at 12:10:14PM +0100, Han-Wen Nienhuys wrote: > > I think passing: > > > > sizes_to_segments(&seglen, NULL, 0); > > > > may make the code more obvious. Unlike system functions like memcpy(), > > we can be assured of whether our functions avoid looking at a > > zero-length array (and size_to_segments does follow that rule). > > > > This function, of course, is nonsense that real code would not do, and > > This test was added because 'real' code did this. In particular, if > you initialize a stack of reftables, the stack has zero elements. The > test was for the following bugfix > > https://github.com/google/reftable/commit/b2e42ecb54e413e494c1fcc13c21e24422645007 > > Changing the array size to 1 here also prevents Valgrind to mark a > regression as a OOB write. Sorry to be unclear. I meant that real code would not allocate a static zero-length array (though it might do so on the heap). I agree that making sure the function behaves in the 0-segment case is quite reasonable. Passing NULL actually makes the test more rigorous IMHO (because it is a strict superset of failure modes, and is something an actual caller might do if it were dynamically growing an array that started at NULL). -Peff
diff --git a/reftable/stack_test.c b/reftable/stack_test.c index c35abd7301..cf2e32a25c 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -579,11 +579,11 @@ static void test_sizes_to_segments(void) static void test_sizes_to_segments_empty(void) { - uint64_t sizes[0]; + uint64_t sizes[1]; int seglen = 0; struct segment *segs = - sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes)); + sizes_to_segments(&seglen, sizes, 0); EXPECT(seglen == 0); reftable_free(segs); }