Chandra Pratap <chandrapratap3519@xxxxxxxxx> 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.
