diff mbox series

[06/10] reftable/generic: move generic iterator code into iterator interface

Message ID 14924604cebe20ac30d291399b0200016fa8b4e3.1723528765.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series reftable: drop generic `reftable_table` interface | expand

Commit Message

Patrick Steinhardt Aug. 13, 2024, 6:24 a.m. UTC
Move functions relating to the reftable iterator from "generic.c" into
"iter.c". This prepares for the removal of the former subsystem.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/generic.c | 107 +--------------------------------------------
 reftable/generic.h |  10 -----
 reftable/iter.c    | 106 ++++++++++++++++++++++++++++++++++++++++++++
 reftable/iter.h    |  27 ++++++++++++
 4 files changed, 134 insertions(+), 116 deletions(-)

Comments

karthik nayak Aug. 13, 2024, 10:04 a.m. UTC | #1
Patrick Steinhardt <ps@pks.im> writes:

[snip]

> +
> +void reftable_iterator_destroy(struct reftable_iterator *it)
> +{
> +	if (!it->ops) {
> +		return;
> +	}
>

I know this commit is to move, but I couldn't help noticing that we
should remove the curly braces here.

Seems like the CI caught it too [1].

> +	it->ops->close(it->iter_arg);
> +	it->ops = NULL;
> +	FREE_AND_NULL(it->iter_arg);
> +}
> +

[snip]

[1]: https://gitlab.com/gitlab-org/git/-/jobs/7563308943
Patrick Steinhardt Aug. 14, 2024, 12:56 p.m. UTC | #2
On Tue, Aug 13, 2024 at 06:04:30AM -0400, karthik nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> [snip]
> 
> > +
> > +void reftable_iterator_destroy(struct reftable_iterator *it)
> > +{
> > +	if (!it->ops) {
> > +		return;
> > +	}
> >
> 
> I know this commit is to move, but I couldn't help noticing that we
> should remove the curly braces here.
> 
> Seems like the CI caught it too [1].

Fair enough, let's remove them.

Patrick
Junio C Hamano Aug. 14, 2024, 4:24 p.m. UTC | #3
karthik nayak <karthik.188@gmail.com> writes:

> Seems like the CI caught it too [1].
>
>> +	it->ops->close(it->iter_arg);
>> +	it->ops = NULL;
>> +	FREE_AND_NULL(it->iter_arg);
>> +}
>> +
>
> [snip]
>
> [1]: https://gitlab.com/gitlab-org/git/-/jobs/7563308943

Looking at it, the suggestions the CI job makes look hit-and-miss.

For examle, this one

-static int stack_compact_range_stats(struct reftable_stack *st,
-				     size_t first, size_t last,
+static int stack_compact_range_stats(struct reftable_stack *st, size_t first,
+				     size_t last,
 				     struct reftable_log_expiry_config *config,
 				     unsigned int flags)

is a degradation of readability.  "first" and "last" pretty
much act as a pair and they read better sitting together next to
each other.  The rewrite is reducing neither the total number of
lines or the maximum column width.

But this one

-static void write_n_ref_tables(struct reftable_stack *st,
-			       size_t n)
+static void write_n_ref_tables(struct reftable_stack *st, size_t n)

is certainly an improvement.

This one

 	struct reftable_record rec = {
 		.type = BLOCK_TYPE_REF,
-		.u = {
-			.ref = *ref
-		},
+		.u = { .ref = *ref },
 	};

is hard to generalize but in this case it made a good suggestion.

If we _expect_ that we will enumerate more members of .u, then the
way originally written (plus trailing comma after the ".ref = *ref")
would be easier to maintain.  But since .u is a union, we won't be
choosing more than one member to initialize by definition, so what
was suggested by the check-style linter is certainly much better.

Thanks.
karthik nayak Aug. 15, 2024, 11:04 a.m. UTC | #4
Junio C Hamano <gitster@pobox.com> writes:

> karthik nayak <karthik.188@gmail.com> writes:
>
>> Seems like the CI caught it too [1].
>>
>>> +	it->ops->close(it->iter_arg);
>>> +	it->ops = NULL;
>>> +	FREE_AND_NULL(it->iter_arg);
>>> +}
>>> +
>>
>> [snip]
>>
>> [1]: https://gitlab.com/gitlab-org/git/-/jobs/7563308943
>
> Looking at it, the suggestions the CI job makes look hit-and-miss.
>

Yup, this is kinda what I was expecting and why I also wanted it to be
allowed to fail. So we have data points on what we should change to make
it better.

> For examle, this one
>
> -static int stack_compact_range_stats(struct reftable_stack *st,
> -				     size_t first, size_t last,
> +static int stack_compact_range_stats(struct reftable_stack *st, size_t first,
> +				     size_t last,
>  				     struct reftable_log_expiry_config *config,
>  				     unsigned int flags)
>
> is a degradation of readability.  "first" and "last" pretty
> much act as a pair and they read better sitting together next to
> each other.  The rewrite is reducing neither the total number of
> lines or the maximum column width.
>

Totally agreed, This is set by `ColumnLimit: 80` and I think most of the
misses I see are because of this. This works with the `Penalties` we set
at the bottom. We need to see what combination works best for us since
our focus would be more on the readability front.

> But this one
>
> -static void write_n_ref_tables(struct reftable_stack *st,
> -			       size_t n)
> +static void write_n_ref_tables(struct reftable_stack *st, size_t n)
>
> is certainly an improvement.
>
> This one
>
>  	struct reftable_record rec = {
>  		.type = BLOCK_TYPE_REF,
> -		.u = {
> -			.ref = *ref
> -		},
> +		.u = { .ref = *ref },
>  	};
>
> is hard to generalize but in this case it made a good suggestion.
>
> If we _expect_ that we will enumerate more members of .u, then the
> way originally written (plus trailing comma after the ".ref = *ref")
> would be easier to maintain.  But since .u is a union, we won't be
> choosing more than one member to initialize by definition, so what
> was suggested by the check-style linter is certainly much better.
>
> Thanks.

Yup these two do look much nicer indeed. There are weird bugs in
clang-format however and I was able to fool it to accept

	struct reftable_record rec = {
		.type = BLOCK_TYPE_REF,
		.u = {
			.ref = *ref },
	};

But this could also be an issue with our config too.

I'll try playing around for an hour today to see if I can find some
improvements we can make to the formatter config.

Thanks
diff mbox series

Patch

diff --git a/reftable/generic.c b/reftable/generic.c
index 28ae26145e..6ecf9b880f 100644
--- a/reftable/generic.c
+++ b/reftable/generic.c
@@ -9,6 +9,7 @@  license that can be found in the LICENSE file or at
 #include "constants.h"
 #include "record.h"
 #include "generic.h"
+#include "iter.h"
 #include "reftable-iterator.h"
 #include "reftable-generic.h"
 
@@ -32,37 +33,6 @@  void reftable_table_init_log_iter(struct reftable_table *tab,
 	table_init_iter(tab, it, BLOCK_TYPE_LOG);
 }
 
-int reftable_iterator_seek_ref(struct reftable_iterator *it,
-			       const char *name)
-{
-	struct reftable_record want = {
-		.type = BLOCK_TYPE_REF,
-		.u.ref = {
-			.refname = (char *)name,
-		},
-	};
-	return it->ops->seek(it->iter_arg, &want);
-}
-
-int reftable_iterator_seek_log_at(struct reftable_iterator *it,
-				  const char *name, uint64_t update_index)
-{
-	struct reftable_record want = {
-		.type = BLOCK_TYPE_LOG,
-		.u.log = {
-			.refname = (char *)name,
-			.update_index = update_index,
-		},
-	};
-	return it->ops->seek(it->iter_arg, &want);
-}
-
-int reftable_iterator_seek_log(struct reftable_iterator *it,
-			       const char *name)
-{
-	return reftable_iterator_seek_log_at(it, name, ~((uint64_t) 0));
-}
-
 int reftable_table_read_ref(struct reftable_table *tab, const char *name,
 			    struct reftable_ref_record *ref)
 {
@@ -152,78 +122,3 @@  uint32_t reftable_table_hash_id(struct reftable_table *tab)
 {
 	return tab->ops->hash_id(tab->table_arg);
 }
-
-void reftable_iterator_destroy(struct reftable_iterator *it)
-{
-	if (!it->ops) {
-		return;
-	}
-	it->ops->close(it->iter_arg);
-	it->ops = NULL;
-	FREE_AND_NULL(it->iter_arg);
-}
-
-int reftable_iterator_next_ref(struct reftable_iterator *it,
-			       struct reftable_ref_record *ref)
-{
-	struct reftable_record rec = {
-		.type = BLOCK_TYPE_REF,
-		.u = {
-			.ref = *ref
-		},
-	};
-	int err = iterator_next(it, &rec);
-	*ref = rec.u.ref;
-	return err;
-}
-
-int reftable_iterator_next_log(struct reftable_iterator *it,
-			       struct reftable_log_record *log)
-{
-	struct reftable_record rec = {
-		.type = BLOCK_TYPE_LOG,
-		.u = {
-			.log = *log,
-		},
-	};
-	int err = iterator_next(it, &rec);
-	*log = rec.u.log;
-	return err;
-}
-
-int iterator_seek(struct reftable_iterator *it, struct reftable_record *want)
-{
-	return it->ops->seek(it->iter_arg, want);
-}
-
-int iterator_next(struct reftable_iterator *it, struct reftable_record *rec)
-{
-	return it->ops->next(it->iter_arg, rec);
-}
-
-static int empty_iterator_seek(void *arg, struct reftable_record *want)
-{
-	return 0;
-}
-
-static int empty_iterator_next(void *arg, struct reftable_record *rec)
-{
-	return 1;
-}
-
-static void empty_iterator_close(void *arg)
-{
-}
-
-static struct reftable_iterator_vtable empty_vtable = {
-	.seek = &empty_iterator_seek,
-	.next = &empty_iterator_next,
-	.close = &empty_iterator_close,
-};
-
-void iterator_set_empty(struct reftable_iterator *it)
-{
-	assert(!it->ops);
-	it->iter_arg = NULL;
-	it->ops = &empty_vtable;
-}
diff --git a/reftable/generic.h b/reftable/generic.h
index 8341fa570e..837fbb8df2 100644
--- a/reftable/generic.h
+++ b/reftable/generic.h
@@ -24,14 +24,4 @@  void table_init_iter(struct reftable_table *tab,
 		     struct reftable_iterator *it,
 		     uint8_t typ);
 
-struct reftable_iterator_vtable {
-	int (*seek)(void *iter_arg, struct reftable_record *want);
-	int (*next)(void *iter_arg, struct reftable_record *rec);
-	void (*close)(void *iter_arg);
-};
-
-void iterator_set_empty(struct reftable_iterator *it);
-int iterator_seek(struct reftable_iterator *it, struct reftable_record *want);
-int iterator_next(struct reftable_iterator *it, struct reftable_record *rec);
-
 #endif
diff --git a/reftable/iter.c b/reftable/iter.c
index a7484aba60..844853fad2 100644
--- a/reftable/iter.c
+++ b/reftable/iter.c
@@ -16,6 +16,43 @@  license that can be found in the LICENSE file or at
 #include "reader.h"
 #include "reftable-error.h"
 
+int iterator_seek(struct reftable_iterator *it, struct reftable_record *want)
+{
+	return it->ops->seek(it->iter_arg, want);
+}
+
+int iterator_next(struct reftable_iterator *it, struct reftable_record *rec)
+{
+	return it->ops->next(it->iter_arg, rec);
+}
+
+static int empty_iterator_seek(void *arg, struct reftable_record *want)
+{
+	return 0;
+}
+
+static int empty_iterator_next(void *arg, struct reftable_record *rec)
+{
+	return 1;
+}
+
+static void empty_iterator_close(void *arg)
+{
+}
+
+static struct reftable_iterator_vtable empty_vtable = {
+	.seek = &empty_iterator_seek,
+	.next = &empty_iterator_next,
+	.close = &empty_iterator_close,
+};
+
+void iterator_set_empty(struct reftable_iterator *it)
+{
+	assert(!it->ops);
+	it->iter_arg = NULL;
+	it->ops = &empty_vtable;
+}
+
 static void filtering_ref_iterator_close(void *iter_arg)
 {
 	struct filtering_ref_iterator *fri = iter_arg;
@@ -181,3 +218,72 @@  void iterator_from_indexed_table_ref_iter(struct reftable_iterator *it,
 	it->iter_arg = itr;
 	it->ops = &indexed_table_ref_iter_vtable;
 }
+
+void reftable_iterator_destroy(struct reftable_iterator *it)
+{
+	if (!it->ops) {
+		return;
+	}
+	it->ops->close(it->iter_arg);
+	it->ops = NULL;
+	FREE_AND_NULL(it->iter_arg);
+}
+
+int reftable_iterator_seek_ref(struct reftable_iterator *it,
+			       const char *name)
+{
+	struct reftable_record want = {
+		.type = BLOCK_TYPE_REF,
+		.u.ref = {
+			.refname = (char *)name,
+		},
+	};
+	return it->ops->seek(it->iter_arg, &want);
+}
+
+int reftable_iterator_next_ref(struct reftable_iterator *it,
+			       struct reftable_ref_record *ref)
+{
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_REF,
+		.u = {
+			.ref = *ref
+		},
+	};
+	int err = iterator_next(it, &rec);
+	*ref = rec.u.ref;
+	return err;
+}
+
+int reftable_iterator_seek_log_at(struct reftable_iterator *it,
+				  const char *name, uint64_t update_index)
+{
+	struct reftable_record want = {
+		.type = BLOCK_TYPE_LOG,
+		.u.log = {
+			.refname = (char *)name,
+			.update_index = update_index,
+		},
+	};
+	return it->ops->seek(it->iter_arg, &want);
+}
+
+int reftable_iterator_seek_log(struct reftable_iterator *it,
+			       const char *name)
+{
+	return reftable_iterator_seek_log_at(it, name, ~((uint64_t) 0));
+}
+
+int reftable_iterator_next_log(struct reftable_iterator *it,
+			       struct reftable_log_record *log)
+{
+	struct reftable_record rec = {
+		.type = BLOCK_TYPE_LOG,
+		.u = {
+			.log = *log,
+		},
+	};
+	int err = iterator_next(it, &rec);
+	*log = rec.u.log;
+	return err;
+}
diff --git a/reftable/iter.h b/reftable/iter.h
index b75d7ac2ac..3b401f1259 100644
--- a/reftable/iter.h
+++ b/reftable/iter.h
@@ -16,6 +16,33 @@  license that can be found in the LICENSE file or at
 #include "reftable-iterator.h"
 #include "reftable-generic.h"
 
+/*
+ * The virtual function table for implementing generic reftable iterators.
+ */
+struct reftable_iterator_vtable {
+	int (*seek)(void *iter_arg, struct reftable_record *want);
+	int (*next)(void *iter_arg, struct reftable_record *rec);
+	void (*close)(void *iter_arg);
+};
+
+/*
+ * Position the iterator at the wanted record such that a call to
+ * `iterator_next()` would return that record, if it exists.
+ */
+int iterator_seek(struct reftable_iterator *it, struct reftable_record *want);
+
+/*
+ * Yield the next record and advance the iterator. Returns <0 on error, 0 when
+ * a record was yielded, and >0 when the iterator hit an error.
+ */
+int iterator_next(struct reftable_iterator *it, struct reftable_record *rec);
+
+/*
+ * Set up the iterator such that it behaves the same as an iterator with no
+ * entries.
+ */
+void iterator_set_empty(struct reftable_iterator *it);
+
 /* iterator that produces only ref records that point to `oid` */
 struct filtering_ref_iterator {
 	struct strbuf oid;