| Message ID | 7955f7983a6d8ef81a572f108b11c7afa93e34fd.1711109214.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: improvements for the `binsearch()` mechanism | expand |
On 24/03/22 01:22PM, Patrick Steinhardt wrote: > The `binsearch()` test is somewhat weird in that it doesn't explicitly > spell out its expectations. Instead it does so in a rather ad-hoc way > with some hard-to-understand computations. > > Refactor the test to spell out the needle as well as expected index for > all testcases. This refactoring highlights that the `binsearch_func()` > is written somewhat weirdly to find the first integer smaller than the > needle, not smaller or equal to it. Adjust the function accordingly. > > While at it, rename the callback function to better convey its meaning. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > reftable/basics_test.c | 55 ++++++++++++++++++++++++------------------ > 1 file changed, 31 insertions(+), 24 deletions(-) > > diff --git a/reftable/basics_test.c b/reftable/basics_test.c > index dc1c87c5df..85c4d1621c 100644 > --- a/reftable/basics_test.c > +++ b/reftable/basics_test.c > @@ -12,40 +12,47 @@ license that can be found in the LICENSE file or at > #include "test_framework.h" > #include "reftable-tests.h" > > -struct binsearch_args { > - int key; > - int *arr; > +struct integer_needle_lesseq { > + int needle; > + int *haystack; > }; This is probably just personal preference, but I think `key` and `arr` in this case are a bit more straightforward. I do like that we rename the args to be more specific. Do we want to also append `_args` to denote that it is an argument set? Maybe `integer_lesseq_args`? > > -static int binsearch_func(size_t i, void *void_args) > +static int integer_needle_lesseq(size_t i, void *_args) > { > - struct binsearch_args *args = void_args; > - > - return args->key < args->arr[i]; > + struct integer_needle_lesseq *args = _args; > + return args->needle <= args->haystack[i]; > } -Justin
On Fri, Mar 22, 2024 at 01:46:56PM -0500, Justin Tobler wrote: > On 24/03/22 01:22PM, Patrick Steinhardt wrote: > > The `binsearch()` test is somewhat weird in that it doesn't explicitly > > spell out its expectations. Instead it does so in a rather ad-hoc way > > with some hard-to-understand computations. > > > > Refactor the test to spell out the needle as well as expected index for > > all testcases. This refactoring highlights that the `binsearch_func()` > > is written somewhat weirdly to find the first integer smaller than the > > needle, not smaller or equal to it. Adjust the function accordingly. > > > > While at it, rename the callback function to better convey its meaning. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > reftable/basics_test.c | 55 ++++++++++++++++++++++++------------------ > > 1 file changed, 31 insertions(+), 24 deletions(-) > > > > diff --git a/reftable/basics_test.c b/reftable/basics_test.c > > index dc1c87c5df..85c4d1621c 100644 > > --- a/reftable/basics_test.c > > +++ b/reftable/basics_test.c > > @@ -12,40 +12,47 @@ license that can be found in the LICENSE file or at > > #include "test_framework.h" > > #include "reftable-tests.h" > > > > -struct binsearch_args { > > - int key; > > - int *arr; > > +struct integer_needle_lesseq { > > + int needle; > > + int *haystack; > > }; > > This is probably just personal preference, but I think `key` and `arr` > in this case are a bit more straightforward. I was trying to make this consistent across all the callsites, and here I personally think that `haystack` and `needle` are well understood by most folks and generic enough. > I do like that we rename > the args to be more specific. Do we want to also append `_args` to > denote that it is an argument set? Maybe `integer_lesseq_args`? Oh, yeah, that's an oversight indeed. Patrick
diff --git a/reftable/basics_test.c b/reftable/basics_test.c index dc1c87c5df..85c4d1621c 100644 --- a/reftable/basics_test.c +++ b/reftable/basics_test.c @@ -12,40 +12,47 @@ license that can be found in the LICENSE file or at #include "test_framework.h" #include "reftable-tests.h" -struct binsearch_args { - int key; - int *arr; +struct integer_needle_lesseq { + int needle; + int *haystack; }; -static int binsearch_func(size_t i, void *void_args) +static int integer_needle_lesseq(size_t i, void *_args) { - struct binsearch_args *args = void_args; - - return args->key < args->arr[i]; + struct integer_needle_lesseq *args = _args; + return args->needle <= args->haystack[i]; } static void test_binsearch(void) { - int arr[] = { 2, 4, 6, 8, 10 }; - size_t sz = ARRAY_SIZE(arr); - struct binsearch_args args = { - .arr = arr, + int haystack[] = { 2, 4, 6, 8, 10 }; + struct { + int needle; + size_t expected_idx; + } testcases[] = { + {-9000, 0}, + {-1, 0}, + {0, 0}, + {2, 0}, + {3, 1}, + {4, 1}, + {7, 3}, + {9, 4}, + {10, 4}, + {11, 5}, + {9000, 5}, }; + size_t i = 0; - int i = 0; - for (i = 1; i < 11; i++) { - size_t res; - - args.key = i; - res = binsearch(sz, &binsearch_func, &args); + for (i = 0; i < ARRAY_SIZE(testcases); i++) { + struct integer_needle_lesseq args = { + .haystack = haystack, + .needle = testcases[i].needle, + }; + size_t idx; - if (res < sz) { - EXPECT(args.key < arr[res]); - if (res > 0) - EXPECT(args.key >= arr[res - 1]); - } else { - EXPECT(args.key == 10 || args.key == 11); - } + idx = binsearch(ARRAY_SIZE(haystack), &integer_needle_lesseq, &args); + EXPECT(idx == testcases[i].expected_idx); } }
The `binsearch()` test is somewhat weird in that it doesn't explicitly spell out its expectations. Instead it does so in a rather ad-hoc way with some hard-to-understand computations. Refactor the test to spell out the needle as well as expected index for all testcases. This refactoring highlights that the `binsearch_func()` is written somewhat weirdly to find the first integer smaller than the needle, not smaller or equal to it. Adjust the function accordingly. While at it, rename the callback function to better convey its meaning. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- reftable/basics_test.c | 55 ++++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 24 deletions(-)