[squid-dev] [CODE] iterating over enums

[Kinkie]

On Thu, Aug 6, 2015 at 12:38 AM, Alex Rousskov <
rousskov at measurement-factory.com> wrote:

> On 08/05/2015 09:33 AM, Kinkie wrote:
>
> > I crossed the topic of enumerating over iterations.
>
> You mean iterating enum[eration]s :-)
>

I have my thinking cap backwards ;)


> > We kind-of follow (C-ish)best
> > practices on that, but that got me wondering if we can do better.
> > I came up with a trinket which on its face looks quite elegant to me,
> > and I'd like to understand with you guys if it makes sense to deploy it
> > in our code.
>
> Maybe, after quite a few changes. It is risky for me to review this
> because the problems in the posted code lie on several very different
> layers, the code is outside Squid context, and I am not even sure what
> [CODE] thread designation means. I hope this does not lead to a long
> discussion, but here are a few things to consider:
>

[CODE] means that it contains code which is not a [PATCH] but meant to
spark a discussion which may or may not result into a patch.
I hope that the discussion will be fruitful, I don't mind if it's long as
long as it doesn't waste people's time (read: it's fruitful)


> * Iterating enums under Squid control ought to be a lot easier, safer,
> and cheaper because we can add begin/end markers *with standard names*
> to each enum. This is important because most iterations iterate
> everything _and_ C++11 range loops always iterate everything. The
> "everything" case is worth focusing on! See attached files for examples
> of how standardized begin/end marks can be used.
>

I agree. We'd need to take care to avoid the anti-pattern of "iterate over
everything but not really everything", but that's exactly the itch I was
trying to scratch, and found unsatisfactorily scratched in my online
research.


> * Your Iterator class is mixing two very different interfaces/APIs: One
> provides begin/end() methods and is required for range loops (I do not
> know how this API is called in the C++ standard). The other one is a
> [partial] iterator API. Do not mix those two APIs! Each of the attached
> files illustrates how to separate them (and there are two ways to
> implement the second/iterator API).
>

Yes, I had unintentionally borrowed from Python where an object sometimes
is its own iterator.
I don't have any particular attachment to the way I coded this - if that's
what you are afraid of when you refer to the long discussion. Anything that
is well self-contained and easy to use will fill the bill for me :)


> * The stand-alone iterator API implementation in
> t-enum-stand-alone-v3.cc allows for nice explicit for-loops and such,
> but those global operators are rather invasive, and I am worried they
> will cause problems in Squid context (as opposed a tiny test file!). I
> recommend that you start with t-enum-v3.cc and then diff the two
> attached files to see changes related to the stand-alone iterator API
> implementation. If you really like stand-alone operator side-effects,
> you may want to investigate whether they would work well in Squid context.
>

Instinctivelty I would go for the class-based approach as it feels less
invasive.
What I understand from the changes you propose is that:
1. instead of having explicit begin/end markers passed as template
arguments you propose to have a convention on begin/end markers.
2. you skip the typedef fixing these parameters in favor of letting the
template parameters appear in the calls. This is quite consistent with an
approach often taken in the stdlib (e.g. std::numeric_limits).

1. looks interesting, especially as an enum missing those markers will fail
to compile. Maybe both approaches can be combined by using default template
arguments, what do you think?
template <typename C, C first = C::enumBegin_, C last_plus_one =
C::enumEnd_>
2. looks verbose to me, but it can be decided on a case-by-case basis,
which can be a good or a bad thing.
3. we could maybe combine the benefits of beautiful range-for and ugly
explicit-for also for shorter ranges by having (in the class-based
variant), on top of what you have already proposed (untested)

template <typename Enum>
class WholeEnum
{
   Enum first, lastplusone;
public:
   WholeEnum (Enum firstValue, Enum lastValuePlusOne) : first(firstValue),
lastplusone(lastValuePlusOne) {}
   Iterator begin() { return Iterator(first); }
   Iterator end() { return Iterator(lastplusone); }
}

this would allow to do
for (auto i : WholeEnum<Numbers>(Numbers::one, Numbers::three)) {
 // do stuff
}

What do you think? Not very kosher maybe, but it should work.

* Avoid static variables in templates, especially when you want to
> instantiate different templates based on such trivial things as range
> boundaries. This is (or used to be?) one of the dark C++ corners; they
> might hurt inlining/performance as well.
>

Ok.


> The attached files are unpolished, but should illustrate the above
> points and how to avoid some of the problems discussed here.
>

It looks to me that I have crossed an itch you feel as well, and this
effort may be useful after all.
Thanks for taking the time!
-- 
    Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150806/bde8996f/attachment.html>