[Variant] Make VariantArray iterable

[friendlymatthew]

Which issue does this PR close?

• Closes [Variant] Remove potential panics when probing VariantArray #8609
• Closes [Variant] Make VariantArray iterable #8612

This PR introduces an Iterator over VariantArray. Since VariantArray does not impl Array, we can't make use of ArrayIter

[kylebarron]

For context this was removed in #8392

[friendlymatthew]

For context this was removed in #8392

Thanks for clarifying

[friendlymatthew]

cc @scovich @alamb

[alamb]

Nice! this looks very useful to me -- thanks @friendlymatthew

[alamb]

thank you for the comments

[alamb]

I double checked the definition of size_hint and this looks good:

https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.size_hint

[scovich]

Very nice! One code cleanup to consider.

[scovich]

Suggested change

	if self.head_i == self.tail_i {
	return None;
	}
	let out = if self.is_null(self.head_i) {
	Some(None)
	} else {
	Some(Some(self.array.value(self.head_i)))
	};
	self.head_i += 1;
	out
	(self.head_i < self.tail_i)
	.then(|| self.value_opt(self.head_i))
	.inspect(|_| self.head_i += 1)

(same story for DoubleEndedIterator below)

[friendlymatthew]

Hi hi, I think using combinators are great, but in this case we're 1) hurting readability and 2) using Option::inspect to mutate state

[scovich]

Agree the inspect call was too clever, but why would this hurt readability?

        (self.head_i < self.tail_i).then(|| {
            let out = self.value_opt(self.head_i);
            self.head_i += 1;
            out
        })

[friendlymatthew]

I think cases like the following are completely valid and very readable. It's simply inspecting the state

(self.head_i < self.tail_i).then(|| self.value_opt(self.head_i)); 

In the closure you posted above, I'm just a bit surprised as a reader that the closure is inspecting the state and mutating it. Plus, I think the following form is simpler to reason through

// base case
if self.head_i == self.tail_i {
    return None;
}

// else
let out = self.value_opt(self.head_i);
self.head_i += 1;

Some(out)

But then again, it's just personal preference. I don't think there's anything wrong with either approach

[alamb]

Thanks @scovich and @friendlymatthew