| Message ID | 20240809111312.4401-3-chandrapratap3519@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: port reftable/readwrite_test.c to the unit testing framework | expand |
Chandra Pratap <chandrapratap3519@gmail.com> writes: > free_names() as defined by reftable/basics.{c,h} frees a NULL > terminated array of malloced strings along with the array itself. > Use this function instead of a for loop to free such an array. Going back to [1/4], the headers included in this test looked like this: -#include "system.h" - -#include "basics.h" -#include "block.h" -#include "blocksource.h" -#include "reader.h" -#include "record.h" -#include "test_framework.h" -#include "reftable-tests.h" -#include "reftable-writer.h" +#include "test-lib.h" +#include "reftable/reader.h" +#include "reftable/blocksource.h" +#include "reftable/reftable-error.h" +#include "reftable/reftable-writer.h" I found this part a bit curious, perhaps because I was not involved in either reftable/ or unit-tests/ development. So I may be asking a stupid question, but is it intended that some headers like "block.h" and "record.h" are no longer included? It is understandable that inclusion of "test-lib.h" is new (and needs to be there to work as part of t/unit-tests/), and the leading directory name "reftable/" added to header files are also justified, of course. But if you depend on "basics.h" and do not include it, that does not sound like the most hygenic thing to do, at least to me. The code changes themselves look good; I can see that the implementation of free_names() in reftable/basics.c safely replaces these loops. There is a slight behaviour difference that names[] that was fed to reftable_iterator_seek_ref() earlier goes away before the iterator is destroyed, but _seek_ref() does not retain the names[0] argument in the iterator object, so that is OK. Thanks.
On Sat, 10 Aug 2024 at 00:27, Junio C Hamano <gitster@pobox.com> wrote: > > Chandra Pratap <chandrapratap3519@gmail.com> writes: > > > free_names() as defined by reftable/basics.{c,h} frees a NULL > > terminated array of malloced strings along with the array itself. > > Use this function instead of a for loop to free such an array. > > Going back to [1/4], the headers included in this test looked like this: > > -#include "system.h" > - > -#include "basics.h" > -#include "block.h" > -#include "blocksource.h" > -#include "reader.h" > -#include "record.h" > -#include "test_framework.h" > -#include "reftable-tests.h" > -#include "reftable-writer.h" > +#include "test-lib.h" > +#include "reftable/reader.h" > +#include "reftable/blocksource.h" > +#include "reftable/reftable-error.h" > +#include "reftable/reftable-writer.h" > > I found this part a bit curious, perhaps because I was not involved > in either reftable/ or unit-tests/ development. So I may be asking > a stupid question, but is it intended that some headers like > "block.h" and "record.h" are no longer included? > > It is understandable that inclusion of "test-lib.h" is new (and > needs to be there to work as part of t/unit-tests/), and the leading > directory name "reftable/" added to header files are also justified, > of course. But if you depend on "basics.h" and do not include it, > that does not sound like the most hygenic thing to do, at least to > me. I think 'basics.{c,h}' in reftable/ is equivalent to 'stdio.h' in a generic C program, it holds fundamental functionalities to be used by other reftable structures and hence, is always (implicitly or explicitly) #included in almost all of the files in reftable/. This test is supposed to focus on reftable's read-write functionalities so it makes sense to explicitly #include only those headers that are directly responsible for those functionalities, namely 'reader.h', 'blocksource.h' and 'reftable-writer.h'. 'reftable-error.h' is thrown in there as well because some tests need to explicitly mention the various error codes and it doesn't make sense to rely on it being #included by others. > The code changes themselves look good; I can see that the > implementation of free_names() in reftable/basics.c safely replaces > these loops. There is a slight behaviour difference that names[] > that was fed to reftable_iterator_seek_ref() earlier goes away > before the iterator is destroyed, but _seek_ref() does not retain > the names[0] argument in the iterator object, so that is OK. > > Thanks.
Chandra Pratap <chandrapratap3519@gmail.com> writes: > On Sat, 10 Aug 2024 at 00:27, Junio C Hamano <gitster@pobox.com> wrote: >> >> Chandra Pratap <chandrapratap3519@gmail.com> writes: >> >> > free_names() as defined by reftable/basics.{c,h} frees a NULL >> > terminated array of malloced strings along with the array itself. >> > Use this function instead of a for loop to free such an array. >> ... > This test is supposed to focus on reftable's read-write functionalities > so it makes sense to explicitly #include only those headers that > are directly responsible for those functionalities, namely 'reader.h', > 'blocksource.h' and 'reftable-writer.h'. 'reftable-error.h' is thrown in > there as well because some tests need to explicitly mention the > various error codes and it doesn't make sense to rely on it being > #included by others. I think we are on the same page. The code explicitly exercises free_names() after this step, and that is exactly why I found it odd to rely on basics.h happen to be included by some other header file(s) we explicitly include. Thanks.
diff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c index 235e3d94c7..e90f2bf9de 100644 --- a/t/unit-tests/t-reftable-readwrite.c +++ b/t/unit-tests/t-reftable-readwrite.c @@ -413,7 +413,6 @@ static void t_table_read_api(void) struct reftable_reader rd = { 0 }; struct reftable_block_source source = { 0 }; int err; - int i; struct reftable_log_record log = { 0 }; struct reftable_iterator it = { 0 }; @@ -432,10 +431,8 @@ static void t_table_read_api(void) check_int(err, ==, REFTABLE_API_ERROR); strbuf_release(&buf); - for (i = 0; i < N; i++) - reftable_free(names[i]); + free_names(names); reftable_iterator_destroy(&it); - reftable_free(names); reader_close(&rd); strbuf_release(&buf); } @@ -498,9 +495,7 @@ static void t_table_read_write_seek(int index, int hash_id) reftable_iterator_destroy(&it); strbuf_release(&buf); - for (i = 0; i < N; i++) - reftable_free(names[i]); - reftable_free(names); + free_names(names); reader_close(&rd); }
free_names() as defined by reftable/basics.{c,h} frees a NULL terminated array of malloced strings along with the array itself. Use this function instead of a for loop to free such an array. Mentored-by: Patrick Steinhardt <ps@pks.im> Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com> --- t/unit-tests/t-reftable-readwrite.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)