[squid-dev] [CODE] iterating over enums

[Kinkie]

Hi Alex,
  it turns out that the itch is real, and this is actually going to be
useful in carrying forward the HttpHeader restructuring I'm doing in
lp:~kinkie/squid/coverity-fixes, to iterate over the list of header types
for various purposes (compacting, etc).
So I went ahead and just implemented it, starting from your observations.
It's a bit more complex that your proposal, as I took the chance to
implement bidirectional forward and reverse iterators.
The attached patch is in changeset 14235 of
lp:~kinkie/squid/coverity-fixes, but since the code is quite well
self-contained I'd like to submit it for audit as a standalone unit; If it
passed I'll merge it to trunk with the code that uses it.

Thanks!
On 08/05/2015 07:58 PM, Kinkie wrote:

> 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.

There are several different things/layers mixed up here:

* If we want to use range loops for enums [in a sane way], then our
enums should have begin/end markers. Many already do, but we will have
to standardize their names and fill the gaps.

* The begin/end parameters should not be template parameters.

* If we need to iterate sub-ranges, then we should add EnumRangeT<Enum>
class with begin/end as constructor parameters (see below).


> 2. you skip the typedef fixing these parameters in favor of letting the
> template parameters appear in the calls.

There are two separate issues/layers here:

* Naming an expression is a good idea, especially if that name is going
to be used multiple times.

* Fewer template parameters are generally better if they can accomplish
the same thing as more template parameters.


> 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_>

I think we should not specify iteration range boundaries as template
parameters.


> 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)

Yes, if sub-range iteration using range loops is needed, then we should
add EnumRangeT<Enum> class that does it. Sorry if I have not mentioned
that explicitly.

Your sketch for that class is fine, but it will be a EnumRangeT class,
separate from simpler WholeEnum, and we would try to define a function
that will create that EnumRangeT object so that we only have to type
"Numbers" zero or two times as illustrated below:

template <typename Enum>
inline
EnumRangeT<Enum> EnumRange(const Enum from, const Enum to) {
    return EnumRangeT<Enum>(from, to);
}


So that instead of:

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

we could do:

  // iterating sub-range of a class enum:
  for (auto i: EnumRange(Numbers::one, Numbers::three)) ...

  // iterating sub-range of a global C enum:
  for (auto i: EnumRange(one, three)) ...

  // iterating a whole enum:
  for (auto i: WholeEnum<Numbers>()) ...


Again, the above range objects can be named (and the first two should be
named if they are used more than once). For example:

  WholeEnum<Numbers> AllNumbers() { return WholeEnum<Numbers>(); }

  EnumRange<Numbers> FooBarNumbers() { return EnumRange(Numbers::foo,
Numbers::bar); }



> It looks to me that I have crossed an itch you feel as well,

No, I am just trying to help you scratch your itch without scarring
Squid code. I do not know whether your itch is real or imaginary.


Cheers,

Alex.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150812/18ce9d13/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: EnumIterator-v1.patch
Type: application/octet-stream
Size: 9348 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150812/18ce9d13/attachment-0001.obj>